Repository navigation
fix(sort): thread --max-memory into the pipeline queue budget - #888
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe sort command maps memory settings to pipeline queue budgets and centralizes ChangesSort chain memory configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change connects the existing memory setting to pipeline queue budgeting with targeted tests and verification; no actionable merge-blocking risk remains beyond normal checks and review. Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
|
@coderabbitai review |
✅ Action performedReviews paused. |
✅ Action performedReview finished.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #888 +/- ##
==========================================
- Coverage 92.98% 92.96% -0.03%
==========================================
Files 298 298
Lines 149836 149872 +36
==========================================
- Hits 139329 139326 -3
- Misses 10507 10546 +39 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The sort-command cutover (#885) hardcoded QueueMemoryOptions::default() (768 MiB/thread) when building the chain, so `fgumi sort --max-memory` bounded only the sorter's in-memory record buffer, not the bytes held in the inter-stage pipeline queues. A small --max-memory (e.g. -m 64K -@ 8) shrank the sort buffer while the queues could still use gigabytes, so peak RSS barely moved. Project the command's single memory knob (--max-memory / --memory-reserve / --memory-per-thread) onto the queue budget via a new queue_memory_options helper, so one flag bounds both budgets. The two totals scale by different thread counts (the sorter by max(threads, sort_threads), the queues by threads); that asymmetry is intentional and documented on the flag. Two deliberate, benign consequences of sharing the flag, documented in code and called out here: - The default queue budget now follows --max-memory's default ("768M" = 768 MB decimal via the size parser), marginally below the former hardcoded 768 MiB, aligning it with the sorter's own default. - A large or `auto` --max-memory does not balloon queue memory: the queue total does not raise the per-stage backpressure marks (512/256 MiB, issue #765), which are what bound in-flight queue bytes. Verified empirically: peak RSS is flat (~380-410 MiB) across -m 768M/auto/4GiB/8GiB at -@ 8 on a 64 GiB host. Extract the ChainSpec construction into a unit-tested build_sort_chain_spec so the wiring (queue budget, stages, threading, write-index sink) is guarded against a silent revert, not only exercised through a full run where a dropped knob is invisible.
0174e3d to
413970e
Compare
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
The sort-command cutover (#885) hardcoded
queue_memory: QueueMemoryOptions::default()(768 MiB/thread) when building the chain, sofgumi sort --max-memorybounded only the sorter's in-memory record buffer, not the bytes held in the inter-stage pipeline queues — a small--max-memory(e.g.-m 64K -@ 8) shrank the sort buffer while the queues could still use gigabytes, so peak RSS barely moved.This projects the command's single memory knob (
--max-memory/--memory-reserve/--memory-per-thread) onto the queue budget via a newqueue_memory_optionshelper, so one flag bounds both budgets. The two totals scale by different thread counts (the sorter bymax(threads, sort_threads)since the sort phase fills the buffer, the queues bythreads— the width of the ingest/output plumbing); that asymmetry is intentional and documented on the flag.Deliberate, benign consequences of sharing the flag
--max-memory's default ("768M"= 768 MB decimal via the size parser), marginally below the former hardcoded 768 MiB, aligning it with the sorter's own default rather than leaving the two at different units.auto--max-memorydoes not balloon queue memory. Raising the queue total does not raise the per-stage backpressure marks (512 MiB / 256 MiB, issue --max-memory above 512 MiB is silently clamped and has no effect #765), which are what actually bound in-flight queue bytes; only a value below them tightens the queues (the point of a small--max-memory).Verification
cargo ci-fmt,cargo ci-lint(pedantic), and the no-default-features check are clean.ChainSpecwiring against a silent revert) and 16test_sort_cutover_parityintegration tests pass.fgumi sort -m 64KiB -@ 8completes with no deadlock, output record count equal to input, correct sort order, and peak RSS 248 MB vs 441 MB at the default (the queue budget is genuinely honored). Peak RSS is flat (~380–410 MiB) across-m 768M/auto/4GiB/8GiBat-@ 8on a 64 GiB host, confirming the per-stage marks bound queue memory rather than the total.The
ChainSpecconstruction was extracted into a unit-testedbuild_sort_chain_specso the wiring (queue budget, stages, threading, write-index sink) is guarded directly, not only exercised through a full run where a dropped knob is invisible.Risk: output changes none;
unsafechanges none and theCLAUDE.mdallowlist is unchanged; memory and queue budgets change, while queue backpressure policy remains unchanged.--max-memorynow controls both the sorter buffer and inter-stage queue budget.queue_memory_optionsapplies distinct thread scaling to each budget and preserves the 768M decimal default.build_sort_chain_speccentralizes chain construction and has unit tests for queue configuration, stages, threading, CRC verification, scheduler settings, and the write-index sink. Tests cover low-memory behavior and chain wiring. Formatting, linting, feature checks, 147 sort unit tests, 16 integration tests, and a smoke test pass.