perf(sort): derive the radix bound inside the first counting pass - #622
Merged
Merged
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughMapped-key radix sorting now derives bounds during first-byte histogram construction, ignores the unmapped sentinel, reuses the histogram during sorting, and derives bounds per chunk. Benchmarks and randomized tests compare fused and externally supplied bounds. ChangesRadix bound refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #622 +/- ##
==========================================
+ Coverage 93.52% 93.53% +0.01%
==========================================
Files 175 175
Lines 105404 105970 +566
==========================================
+ Hits 98574 99123 +549
- Misses 6830 6847 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nh13
force-pushed
the
nh/perf-sort-fused-radix-bound
branch
from
July 21, 2026 06:26
3afb192 to
ebf0736
Compare
nh13
force-pushed
the
nh/perf-sort-radix-key-borrow
branch
from
July 21, 2026 09:03
eba0243 to
f8a371b
Compare
nh13
force-pushed
the
nh/perf-sort-fused-radix-bound
branch
from
July 21, 2026 20:43
ebf0736 to
8c8f6fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-sort/src/inline.rs (1)
1625-1643: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc comment contradicts the fused implementation below it.
This doc block still describes the pre-fusion behavior ("Scans
refsfor the maximum key, then defers toradix_sort_record_refs_with_max"), but the function body (lines 1644-1670) no longer scans-then-defers — it fuses the bound derivation into the byte-0 counting pass and callsradix_sort_sizeddirectly. This directly contradicts the updated doc onradix_sort_record_refs_with_max(lines 1710-1719), which correctly says the bound is now derived "inside its first counting pass." The stale "at the cost of upholding its precondition" advice about calling_with_maxdirectly to "skip the scan" is also now misleading, since there's no separate scan step left to skip.In a file explicitly flagged as output-identity-critical with approved unsafe hot paths, stale docs like this increase the risk of a future maintainer misunderstanding the fused algorithm while modifying it.
📝 Proposed doc fix
-/// Scans `refs` for the maximum key, then defers to -/// [`radix_sort_record_refs_with_max`]. A caller that already holds a valid -/// bound — such as a chunked sort that derived one for the whole slice — can -/// call that directly to skip the scan, at the cost of upholding its -/// precondition. +/// Derives the maximum non-sentinel key and the byte-0 histogram in a single +/// fused counting pass, then dispatches to the sized radix implementation. +/// See [`radix_sort_record_refs_with_max`] for the (non-production) entry +/// point that accepts a caller-supplied bound instead. /// /// The scan deliberately ignores `u64::MAX` keys (the unmapped coordinate🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fgumi-sort/src/inline.rs` around lines 1625 - 1643, Update the documentation for radix_sort_record_refs to describe the fused maximum-bound derivation performed during the first byte-counting pass and its direct radix_sort_sized path. Remove the outdated claim that it scans first and delegates to radix_sort_record_refs_with_max, including the advice about skipping that scan; retain accurate sentinel-handling and stability guarantees.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/fgumi-sort/src/inline.rs`:
- Around line 2991-3029: Update the test data generation in
test_fused_bound_matches_externally_supplied_bound so the wide_keys case
produces keys requiring 4–8 radix bytes, rather than only changing n. Keep the
other cases’ existing key distribution and sentinel behavior unchanged, and
ensure the wide-key values still exercise fused bound derivation while remaining
valid for the sorting assertions.
---
Outside diff comments:
In `@crates/fgumi-sort/src/inline.rs`:
- Around line 1625-1643: Update the documentation for radix_sort_record_refs to
describe the fused maximum-bound derivation performed during the first
byte-counting pass and its direct radix_sort_sized path. Remove the outdated
claim that it scans first and delegates to radix_sort_record_refs_with_max,
including the advice about skipping that scan; retain accurate sentinel-handling
and stability guarantees.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16a9af5c-eda8-4477-95ca-cb5d3222e595
📒 Files selected for processing (2)
benches/core_functions.rscrates/fgumi-sort/src/inline.rs
Member
Author
|
Addressed both CodeRabbit findings:
The failing |
nh13
force-pushed
the
nh/perf-sort-fused-radix-bound
branch
from
July 21, 2026 21:28
8c8f6fd to
befcde7
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
nh13
force-pushed
the
nh/perf-sort-fused-radix-bound
branch
from
July 22, 2026 22:41
befcde7 to
407121d
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/fgumi-sort/src/inline.rs (1)
461-473: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale doc comment elsewhere now contradicts this per-chunk bound change.
The doc comment on
test_par_sort_into_chunks_handles_unmapped_sentinel(further down, ~line 3242) still reads "par_sort_into_chunksderives one bound for the whole buffer and shares it across every chunk" — that's the pre-fusion design this PR replaces. The new comment here correctly states each chunk derives its own bound independently, so the test doc is now factually wrong and will mislead the next reader into thinking there's a shared-bound invariant to preserve.📝 Suggested fix
- /// `par_sort_into_chunks` derives one bound for the whole buffer and shares - /// it across every chunk, so the sentinel exclusion has to hold there too: - /// each chunk must come back sorted and no unmapped record may be lost, on - /// both the single-threaded early-return path and the multi-threaded path - /// (which drain `refs` through different branches of the macro). + /// `par_sort_into_chunks` now has each chunk derive its own bound inside + /// its byte-0 counting pass, so the sentinel exclusion has to hold + /// per-chunk: each chunk must come back sorted and no unmapped record may + /// be lost, on both the single-threaded early-return path and the + /// multi-threaded path (which drain `refs` through different branches of + /// the macro).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fgumi-sort/src/inline.rs` around lines 461 - 473, Update the documentation for test_par_sort_into_chunks_handles_unmapped_sentinel to describe that par_sort_into_chunks derives the bound independently for each chunk, removing the outdated claim that one whole-buffer bound is shared across chunks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/fgumi-sort/src/inline.rs`:
- Around line 461-473: Update the documentation for
test_par_sort_into_chunks_handles_unmapped_sentinel to describe that
par_sort_into_chunks derives the bound independently for each chunk, removing
the outdated claim that one whole-buffer bound is shared across chunks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a60e875-0e0e-400f-aeee-09946260b936
📒 Files selected for processing (2)
benches/core_functions.rscrates/fgumi-sort/src/inline.rs
Builds on the previous commit, which sized radix passes from the largest
non-sentinel key via a standalone scan of `refs`. That scan is a second
traversal of an array the sort is about to walk anyway.
The byte-0 counting pass already loads every `sort_key` to build its histogram,
and it runs before any element is scattered, so the bound can be derived there
-- for the cost of a compare per record rather than another pass over memory --
and is still known in time to size the run. The histogram that pass built is
handed to its own scatter, so nothing is counted twice.
Measured on an otherwise idle c6a.4xlarge, criterion 100 samples, confidence
intervals within +/-0.2%:
records full width scan fused tracked
1M 23.76 M/s 28.48 29.40 29.26
8M 20.88 M/s 24.65 25.40 25.61
Fusing is worth 3.0-3.2% over the scan, and lands within 1% of the incremental
`max_sort_key` tracking that the previous commit deliberately dropped -- so it
recovers that win without a field to keep in sync across pushes, clears and
drains, without the reset-before-macro ordering hazard, and, most importantly,
without a bound that is only checked by a `debug_assert` and therefore silently
mis-sorts if it ever drifts in a release build. The bound is recomputed from the
data on every sort and cannot disagree with the keys it is sorting.
`par_sort_into_chunks` now lets each chunk derive its own bound rather than
sharing a whole-buffer one. That removes the last production caller of the
unchecked `radix_sort_record_refs_with_max`, which is retained for benchmarks
and tests that need to pin a specific width, and documented as such. A per-chunk
bound is also never wider than a shared one, so a chunk with narrow keys runs
fewer passes.
Tested for output identity against an externally supplied bound across mixed,
sentinel-free, sentinel-heavy, threshold-boundary and wide-key inputs, asserting
the stable tie-break order as well as the ordering. Verified non-vacuous by
corrupting the reused histogram, which fails every case.
nh13
force-pushed
the
nh/perf-sort-fused-radix-bound
branch
from
July 23, 2026 03:06
407121d to
45811b2
Compare
Merged
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself: six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. Everything landed so far -- `SortMergeSlot` and its loom model (#718), the arena pool and block-offset planner (#719) -- came out of the same upstream commit and was split off ahead of this one; this is its remainder. `chunk_sorter.rs` (coordinate + template chunk sorters), `ref_sort.rs` (the parallel coordinate ref sort), `template_arena.rs` (the template-coordinate accumulator), `spill_block.rs` and `spill_block_reader.rs` (the block-granular spill codec), and `sync_spill_writer.rs` (the synchronous inline spill compressor). These could not land separately: `chunk_sorter`, `ref_sort`, and `template_arena` form a dependency cycle among themselves and all reach into the rewritten `external.rs`, while the three spill modules need `external.rs` and `worker_pool.rs` -- and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 of them on `external.rs` alone -- so the changes below are places where both sides moved and neither could simply win. - `keys.rs` gained an inline `SmallVec` name buffer upstream, while main independently added a `pos: u32` ingest position to make name+flags a total order (#621, which is what lets the chunk sort be unstable). Both are kept: the key is now `NameBuf` + flags + pos. Taking either side alone would have dropped an optimization or reintroduced nondeterminism. `position_fits_in_existing_key_padding` asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; measured, `NameBuf` is 40 and the key is 48. The test's *claim* -- that `pos` is free -- still holds, since name+flags alone already round up to 48, so the expectation and its derivation were updated rather than the assertion weakened, and the padding itself is now pinned. - `keys.rs::header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive intact. Verified by diffing the function against the base rather than by inspection. - `inline.rs` set `RecordBuffer::max_sort_key`, a field main removed when it started deriving the radix bound inside the first counting pass (#622). Both sides had optimized the same thing; main's needs no stored field, so the reset is dropped. - `lib.rs` re-exports are a union, not either side. The engine's export list drops `format_thread_counts` (main-only, from #691, and it lives in the very `external.rs` being rewritten), `SortMergeReader`, and the `reader.rs` entry points including `open_raw_bam_record_reader_with_header`. The root crate imports those, so they are kept alongside the engine's new `MergeDriver` / `open_spill_slot` / arena exports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It re-targets the unpark handle for the streaming sort path, needs the `ArcSwap` field type from an upstream change main never took, and has no callers anywhere -- its own doc says its production caller was retired. Porting it would have meant adding a dependency to support dead code for a path that does not exist here. - `smallvec` is declared crate-local rather than workspace-inherited: `keys.rs` is its only user, and the root manifest reserves `[workspace.dependencies]` for crates shared by two or more members. Three doc claims that were true when written are now false, and are corrected here rather than left to rot: - `merge_slots.rs` said `SortMergeSlot` has no callers. It has two now (`open_spill_slot`, `MergeDriver::from_slots`), both reachable only from tests. - `worker_pool.rs` carried the source branch's claim that the pool path "no longer drives any production sort". In this tree it is the ONLY thing that does -- `fgumi sort` and `fgumi merge` both construct a `RawExternalSorter`. The end state it describes needs the typed-step `SortMerge` consumer, which lands with `fgumi-pipeline-io`. - `docs/design/sort-phase2-unification-deferral.md` said `external.rs::MergeDriver` does not exist on this branch. It does now. CLAUDE.md gains the `ref_sort.rs` unsafe allowlist entry (the `segmented_buf.rs` one arrived with the arena pool), plus a note reconciling the counts: a raw grep finds 22 `#[allow(unsafe_code)]` sites in the crate against the 7 production sites the allowlist names, and the difference is entirely tests exercising already-approved APIs. Verified: `ci-fmt`, `ci-lint`, `ci-doc`, `ci-tag-literals` clean, 7,903 tests pass, 5/5 loom models pass, and `--no-default-features` / `--all-features` both build. Note that the source branch does not build its own workspace at this commit -- `fgumi-pipeline-io` there imports four `*SortStream` symbols the engine removed -- so this is the first tree in which the arena engine compiles against its consumers. Eight public doc comments linked to private items -- `TemplateRecordBuffer` and its `par_sort`, `verify_dropped_lanes`, `Self::create_temp_dirs`, `RecordBuffer::drain_into_single_chunk`, `PARALLEL_SORT_THRESHOLD` (twice), and `SyncSpillWriter`. Making the modules public is what exposed them. `cargo ci-doc` only warns; the `docs` job sets `RUSTDOCFLAGS=-D warnings` and fails. None is a visibility leak -- `cargo ci-lint` reports no `private_interfaces`, so no private type appears in a public signature -- so each is prose naming an internal, and the fix is to drop the link and keep the name in backticks rather than widen the API to satisfy rustdoc. Adapted to the arena pool's RAII acquisition. `ArenaPool::try_acquire` now hands back `PooledSegmentedBuf` rather than a bare `SegmentedBuf`, because a slot dropped without the wrapper is retired permanently and the pool then wedges. `RecordBuffer::data` holds the wrapper for the whole fill rather than only from the drain onward, which is what actually closes that window: an arena acquired here previously sat bare inside the buffer until `drain_into_pooled_chunk`, so any early return over that span leaked it. `install_arena` takes the wrapper, and the drain moves it out instead of re-wrapping — which made `drain_into_pooled_chunk`'s `pool` parameter redundant, so it is gone. `block_offsets.rs` is dropped with the pool commit and is not re-added here: it re-derived `SegmentedBuf`'s offset arithmetic, claimed a seal parity with `memory_usage() >= memory_limit` it did not have, and had no caller in this commit or on the source branch.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 9, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 10, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 19, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 19, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
nh13
added a commit
that referenced
this pull request
Aug 23, 2026
The engine itself -- the last and largest slice of P3. Six modules plus the `external.rs`, `worker_pool.rs`, `inline.rs`, and `keys.rs` rewrites that carry them. `SortMergeSlot` and its loom model (#718) and the arena pool and block-offset planner (#719) came out of the same upstream commit and were split off ahead of this one; this is its remainder. `chunk_sorter.rs`, `ref_sort.rs`, `template_arena.rs`, `spill_block.rs`, `spill_block_reader.rs`, and `sync_spill_writer.rs` could not land separately: the first three form a dependency cycle among themselves and all reach into the rewritten `external.rs`, the last three need `external.rs` and `worker_pool.rs`, and `external.rs` in turn depends on the new modules. This is a three-way merge, not a port. The source commit predates 30 commits of `fgumi-sort` work on main -- 18 on `external.rs` alone -- so wholesale copying would have silently reverted them. Where both sides moved: - `keys.rs` takes both improvements to the queryname key: upstream's inline `SmallVec` name buffer and main's `pos: u32` ingest position (#621), which is what makes name+flags a total order and lets the chunk sort be unstable. Taking either side alone drops an optimization or reintroduces nondeterminism. The padding test asserted 32 bytes on the reasoning that a `Vec<u8>` is 24; the key is larger, but the test's claim -- that `pos` is free -- still holds because name+flags alone already round up. The expectation and its derivation are corrected and the padding pinned directly. - `header_ss_tag` is byte-identical to main's, so the `@HD SS` spelling fixes (#514, #567) survive. Verified by diffing against the base. - `inline.rs` drops the `RecordBuffer::max_sort_key` reset: both sides optimized the same thing and main's derives the radix bound inside the first counting pass (#622), needing no stored field. - `lib.rs` re-exports are a union. The engine's list dropped `format_thread_counts` (#691), `SortMergeReader`, and the `reader.rs` entry points, all of which the root crate imports. - `worker_pool.rs::set_main_thread` is dropped rather than ported. It needs an `ArcSwap` field type from an upstream change main never took and has no callers; its own doc says its production caller was retired. `smallvec` is a crate-local dependency, not workspace-inherited -- `keys.rs` is its only user -- with the `const_generics` feature, since the default `Array` impls cover 1..=32 then 36, 64, and the inline name capacity is 44. That capacity fits 100% of read names in every local benchmark dataset; on native Illumina names (37-41 bytes) it is 9.5% less CPU and 4.4% less peak RSS end-to-end, against ~4% more RSS on short SRA-normalized names. Doc claims that were true when written and are not now: `merge_slots.rs` said `SortMergeSlot` has no callers (it has two, both test-reachable); `worker_pool.rs` said the pool path no longer drives any production sort (here it is the only thing that does, until the typed-step `SortMerge` consumer lands with `fgumi-pipeline-io`); and the Phase-2 unification deferral doc said `external.rs::MergeDriver` does not exist. CLAUDE.md gains the `ref_sort.rs` and `segmented_buf.rs` unsafe allowlist entries, plus a note reconciling a raw grep's 22 `#[allow(unsafe_code)]` sites against the 7 production sites the allowlist names -- the difference is entirely tests exercising already-approved APIs.
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #606 — review that first; this PR's diff is only the last commit.
#606 sizes radix passes from the largest non-sentinel key, found via a standalone scan of
refs. It also deliberately dropped upstream's incrementalmax_sort_keytracking, which avoided that scan and measured 4% faster, on the grounds that the tracking needs a field kept in sync across three reset sites, has a reset-before-macro ordering hazard, and guards its bound with adebug_assert— meaning a drift in a release build silently mis-sorts.This recovers that 4% without reintroducing any of it.
How
The byte-0 counting pass already loads every
sort_keyto build its histogram, and it runs before any element is scattered. So the bound can be derived right there — for a compare per record rather than another traversal of memory — and is still known in time to size the run. The histogram that pass built is handed to its own scatter, so nothing is counted twice.There is no invariant to check, in release or otherwise: the bound is recomputed from the data on every sort and cannot disagree with the keys it is sorting.
Measured
Idle
c6a.4xlarge, criterion 100 samples, confidence intervals within ±0.2%:Fusing is worth +3.0–3.2% over the scan and lands within 1% of the tracked version — i.e. it recovers the win that motivated the tracking, with none of the machinery. The benchmark carries all four arms so the claim is reproducible. A second independent run on a fresh c6a reproduced this (fused 30.15 vs tracked 30.32 at 1M; 26.33 vs 26.13 at 8M).
Cross-architecture caveat
The "within 1%" result is x86. On an M2 Max the same benchmark puts tracking ~7% ahead of fusing (102.8 vs 96.2 M/s at 1M, intervals ±0.7%) — plausibly because folding the max into the counting loop inhibits some unrolling there. Both are far ahead of the scan on that machine.
So the honest claim is: fusing equals tracking on x86 and trails it by ~7% on Apple Silicon, while requiring no field, no reset sites, and no release-invisible bound. Given the in-memory sort is a small share of coordinate wall time, I think that trade still favours fusing — but the parity claim should not be read as architecture-independent.
Also
par_sort_into_chunksnow lets each chunk derive its own bound instead of sharing a whole-buffer one. That removes the last production caller of the uncheckedradix_sort_record_refs_with_max, which is retained for benchmarks and tests that need to pin a width (passingu64::MAXto force full 8 passes) and documented as not-for-production. A per-chunk bound is also never wider than a shared one, so a chunk with narrow keys runs fewer passes.Testing
Output identity against an externally supplied bound across mixed, sentinel-free, sentinel-heavy, threshold-boundary and wide-key inputs — asserting the stable tie-break order, not just the ordering. Verified non-vacuous by corrupting the reused histogram, which fails all five cases.
cargo ci-test(5578 tests),cargo ci-fmt,cargo ci-lint,RUSTDOCFLAGS="-D warnings" cargo ci-docpass.Summary by CodeRabbit
Performance
Tests
Reliability