Repository navigation
revert(sort): restore --max-memory default to 768M - #245
Conversation
On c7g.4xlarge benchmarks, --max-memory=auto regresses WES/WGS sort runtimes (up to +54% on WES coordinate t8) and uses 1.4-3.4x more RSS than 768M. Root cause is page-cache eviction: when the sort budget approaches total RAM, the OS cannot keep phase-1 spill files hot and phase 2 re-reads them from NVMe instead of the page cache. auto remains available as an opt-in value. A follow-up will introduce a hybrid sort that can use the extra memory safely, after which the default will flip back to auto.
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
==========================================
- Coverage 89.04% 88.94% -0.11%
==========================================
Files 114 119 +5
Lines 55304 57819 +2515
==========================================
+ Hits 49247 51425 +2178
- Misses 6057 6394 +337 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/commands/sort.rs (1)
205-205: Add a regression test for the clap default.To prevent future flips/regressions, add one parser-level test asserting
Sortdefaultsmax_memoryto"768M".Proposed test
#[test] fn test_parse_memory_auto() { @@ } + +#[test] +fn test_clap_default_max_memory_is_768m() { + let sort = <Sort as clap::Parser>::parse_from([ + "fgumi", + "sort", + "-i", + "in.bam", + "-o", + "out.bam", + ]); + assert_eq!( + sort.max_memory, + parse_memory("768M").expect("parse_memory should succeed for 768M") + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/sort.rs` at line 205, Add a parser-level unit test that verifies the Clap default for Sort::max_memory remains "768M": write a test that constructs/parses the Sort command with no arguments (e.g., via clap's parse_from or deriving Parser for Sort) and assert that the resulting Sort.max_memory (or the field name max_memory) equals the string "768M" (or the parsed Memory type's original string if using a wrapper) to lock in the attribute #[arg(..., default_value = "768M", value_parser = parse_memory)] behavior; place the test alongside other command tests and reference the Sort struct, the max_memory field, and parse_memory in the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/commands/sort.rs`:
- Line 205: Add a parser-level unit test that verifies the Clap default for
Sort::max_memory remains "768M": write a test that constructs/parses the Sort
command with no arguments (e.g., via clap's parse_from or deriving Parser for
Sort) and assert that the resulting Sort.max_memory (or the field name
max_memory) equals the string "768M" (or the parsed Memory type's original
string if using a wrapper) to lock in the attribute #[arg(..., default_value =
"768M", value_parser = parse_memory)] behavior; place the test alongside other
command tests and reference the Sort struct, the max_memory field, and
parse_memory in the assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e188877-e6b0-43a2-b176-24a0cd47b050
📒 Files selected for processing (1)
src/lib/commands/sort.rs
This branch was previously deployed
1 participant
Summary
fgumi sort --max-memorydefault fromautoback to768M(samtools-compatible)autoremains available as an opt-in valueWhy
On c7g.4xlarge benchmarks,
--max-memory=autoregresses WES/WGS sort runtimes (up to +54% on WES coordinate t8) and uses 1.4-3.4x more RSS than768M. Root cause is page-cache eviction: when the sort budget approaches total RAM, the OS cannot keep phase-1 spill files hot, and phase 2 re-reads them from NVMe instead of from the page cache. Local dev machines with abundant RAM and fast NVMe masked this during the original PR #236 validation.This restores the previous default as an immediate fix. A follow-up will introduce a hybrid sort (small per-cycle sorts + in-memory chunk pool) that can use the extra memory safely; once that lands and is verified on the full c7g matrix, the default will flip back to
auto.Test plan
cargo ci-test(2296 passed)cargo ci-fmtcargo ci-lint