Skip to content

fix(sort): scale the per-thread memory budget by the sort phase - #692

Merged
nh13 merged 1 commit into
mainfrom
tf_sort_memory_threads
Aug 2, 2026
Merged

nh13 merged 1 commit into
mainfrom
tf_sort_memory_threads

Conversation

@tfenne

@tfenne tfenne commented Aug 1, 2026

Copy link
Copy Markdown
Member

Stacked on #691 — please review/merge that one first; this PR targets its branch, so the diff here is just the memory change.

--memory-per-thread multiplied --max-memory by --threads alone. Since --sort-threads defaults to --threads and not the reverse, a run that set only the per-phase override got the one-thread budget while sorting with eight workers:

$ fgumi sort -i in.bam -o out.bam --max-memory 100M --sort-threads 8
INFO  Max memory: 95.4 MiB (95.4 MiB/thread x 1 threads)
INFO  Threads: sort 8, merge 1

The budget sizes the in-memory accumulation buffer, which the sort phase fills, so it now scales by that phase:

INFO  Max memory: 762.9 MiB (95.4 MiB/thread x 8 threads)

Why max(--threads, --sort-threads) and not just --sort-threads

Scaling strictly by the sort phase would be a silent regression for the pattern --sort-threads exists to serve. -@ 32 --sort-threads 4 is the documented way to cede cores to an upstream producer while keeping the merge wide; under sort-phase-only scaling that run's buffer drops from 32× to 4× — an eightfold cut, trading a scheduling hint for a throughput cliff. Taking the larger of the two raises the budget exactly where --threads under-counts and never lowers it, so no existing invocation resolves less memory than it does today.

Scope

  • --threads 0 still reaches resolve_memory_budget's rejection rather than being clamped up by the new helper.
  • --memory-per-thread false is unaffected (still a fixed total).
  • The Max memory log line and the --max-memory auto initial-capacity cap both follow the same count, so neither can disagree with the resolved budget.
  • Only fgumi sort is touched. QueueMemoryOptions (filter/clip/correct) has a single thread count with no sort/merge phases, so it needs no equivalent — and the README/performance-tuning memory notes describe that pipeline-queue path, not this one.

The helper re-derives the sort-phase fallback because the budget is needed before the sorter exists (so it can't read phase1_threads off it, as the Threads: line in #691 does). A test asserts the two definitions agree, so they can't drift.

Tests: 7 cases over the multiplier (including both zero cases), 4 drift-guard cases, and two end-to-end tests asserting the logged budget — one for the bug, one pinning the -@ 32 --sort-threads 4 shape against the regression above. I confirmed the end-to-end tests fail before the fix, at 95.4 MiB (... x 1 threads).

ci-fmt/ci-lint clean. ci-test is 6905 passed / 3 failed — the same test_input_source_matrix::declared_{sam,stdin}_support_* trio that fails on a pristine main (they diff R-generated simplex_qc.pdf byte-for-byte and R stamps /CreationDate into it), as noted in #690 and #691.

@tfenne
tfenne requested a review from nh13 August 1, 2026 14:01
@tfenne
tfenne requested a review from nh13 as a code owner August 1, 2026 14:01
@tfenne
tfenne temporarily deployed to github-actions August 1, 2026 14:01 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Aug 1, 2026 •

Copy link
Copy Markdown

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nh13

nh13 commented Aug 1, 2026

Copy link
Copy Markdown
Member

@coderabbitai pause

@coderabbitai

coderabbitai Bot commented Aug 1, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews paused.

@codecov

codecov Bot commented Aug 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.55172% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.93%. Comparing base (521e88d) to head (a628610).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/commands/sort.rs 96.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #692      +/-   ##
==========================================
- Coverage   93.95%   93.93%   -0.02%     
==========================================
  Files         178      178              
  Lines      108086   108109      +23     
==========================================
+ Hits       101552   101553       +1     
- Misses       6534     6556      +22     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nh13
nh13 force-pushed the tf_sort_thread_logging branch from 8d95c13 to 6eec80e Compare August 2, 2026 00:02
@nh13
nh13 force-pushed the tf_sort_memory_threads branch from e68abb3 to 53ac577 Compare August 2, 2026 00:02
Base automatically changed from tf_sort_thread_logging to main August 2, 2026 00:08
`--memory-per-thread` multiplied `--max-memory` by `--threads` alone. Since
`--sort-threads` defaults to `--threads` rather than the reverse, a run that
set only the per-phase override got the one-thread budget while sorting with
eight workers:

    $ fgumi sort -i in.bam -o out.bam --max-memory 100M --sort-threads 8
    INFO  Max memory: 95.4 MiB (95.4 MiB/thread x 1 threads, from --threads)

The budget sizes the in-memory accumulation buffer, which the sort phase
fills, so it now scales by that phase:

    INFO  Max memory: 762.9 MiB (95.4 MiB/thread x 8 threads, from --sort-threads)

Take the larger of `--threads` and `--sort-threads` rather than the sort
phase alone. Lowering only the sort phase (`-@ 32 --sort-threads 4`) is the
documented way to cede cores to an upstream producer while keeping the merge
wide; scaling strictly by the sort phase would cut that run's buffer eightfold
and trade a scheduling hint for a throughput cliff. The larger of the two
raises the budget in the case `--threads` under-counts and never lowers it, so
no existing invocation resolves less memory than before.

That makes `--sort-threads` a memory knob as well as a scheduling one, so the
three docs that said otherwise are corrected: its own help no longer claims it
"only changes scheduling", `--threads` is described as the floor for the
multiplier rather than the multiplier, and `--memory-per-thread` names the pair
it scales by. `--merge-threads` is left alone -- the merge phase never reads
`memory_limit`, so that flag really is scheduling-only.

#691 added a `, from --threads` suffix to the `Max memory` line so it could not
be misread against the per-phase `Threads:` line below it. That attribution is
now conditional, since the count can come from either flag, so it reports
whichever one supplied it.

`--threads 0` still reaches `resolve_memory_budget`'s rejection rather than
being clamped, and `--memory-per-thread false` is unaffected. The `Max memory`
log line and the `--max-memory auto` initial-capacity cap both follow the same
count, so neither can disagree with the budget. The auto cap moves into
`Sort::auto_initial_capacity` so it is unit-testable, including its overflow
guard.

Testing: the `Max memory` log line is printed from the same local that feeds
`resolve_memory_budget`, so an integration test asserting on it cannot tell a
correctly wired budget from one that logs the sort-phase count and resolves
`--threads`. `test_memory_budget_threads_resolves_the_scaled_budget` pins the
resolved byte count independently. The sorter-agreement test takes the expected
phase-1 count from its case table instead of re-applying the implementation's
own `max`, which could not have detected an engine fallback resolving below
`--threads`.
@nh13
nh13 force-pushed the tf_sort_memory_threads branch from 53ac577 to a628610 Compare August 2, 2026 00:12
@nh13
nh13 temporarily deployed to github-actions August 2, 2026 00:12 — with GitHub Actions Inactive
@nh13
nh13 merged commit 2fce79c into main Aug 2, 2026
14 checks passed
@nh13
nh13 deleted the tf_sort_memory_threads branch August 2, 2026 00:17
@nh13 nh13 mentioned this pull request Aug 2, 2026
@nh13 nh13 mentioned this pull request Aug 15, 2026

This branch was previously deployed

1 inactive deployment
github-actions — a628610d Deployed Aug 2, 2026 by nh13 via coverage #3223
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants