feat(sort): add the arena pool and block-offset planner - #719
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Walkthrough
ChangesSegmented arena allocation and reuse
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ArenaPool
participant PooledSegmentedBuf
participant SegmentedBuf
Caller->>ArenaPool: try_acquire
ArenaPool->>SegmentedBuf: reuse or allocate arena
ArenaPool-->>Caller: return pooled wrapper
Caller->>PooledSegmentedBuf: write through DerefMut
PooledSegmentedBuf->>ArenaPool: return buffer on drop
ArenaPool->>SegmentedBuf: reset_for_reuse
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
Member
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main-runall #719 +/- ##
==============================================
Coverage ? 93.99%
==============================================
Files ? 208
Lines ? 119092
Branches ? 0
==============================================
Hits ? 111943
Misses ? 7149
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/lib.rs`:
- Around line 44-46: Demote the unused public arena-pool API to crate
visibility: update crates/fgumi-sort/src/lib.rs lines 44-46 to make arena_pool
crate-private, remove its re-export at crates/fgumi-sort/src/lib.rs line 167,
and change ArenaPool, PooledSegmentedBuf, and their methods in
crates/fgumi-sort/src/arena_pool.rs lines 61-71 to pub(crate), including making
free_len consistently crate-private. Leave segmented_buf private and preserve
the existing signatures under the crate-only visibility.
In `@crates/fgumi-sort/src/segmented_buf.rs`:
- Around line 230-232: Add crates/fgumi-sort/src/segmented_buf.rs to the
approved-unsafe inventory in CLAUDE.md, documenting that zero-fill through
reserve_contiguous plus extend_in_place is unsuitable for the parallel-inflate
admit path because writes are serialized single-threadedly. Leave the existing
grow_uninit and slice_mut safety comments and attributes unchanged.
- Around line 766-773: Update the comment in
slice_mut_concurrent_disjoint_writes to remove the claim that the test is
checked for data races under Miri, since the current Miri job does not cover
fgumi-sort. Do not change the test behavior or add unrelated coverage changes.
🪄 Autofix
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: 35172338-a90d-4a82-9aa9-2f00906a62b5
📒 Files selected for processing (4)
crates/fgumi-sort/src/arena_pool.rscrates/fgumi-sort/src/block_offsets.rscrates/fgumi-sort/src/lib.rscrates/fgumi-sort/src/segmented_buf.rs
nh13
force-pushed
the
nh/runall-06-arena-leaf
branch
from
August 6, 2026 07:45
b96224a to
ca7a9f9
Compare
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.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/arena_pool.rs`:
- Around line 61-68: Update ArenaPool::try_acquire to return PooledSegmentedBuf
rather than a raw SegmentedBuf, wrapping both reused and newly allocated buffers
in a pool-owned lease. Make PooledSegmentedBuf::pooled private and add DerefMut
so callers can mutate the leased buffer during the fill phase; ensure only
leases created by the pool can return buffers through release.
- Around line 45-50: Update ArenaPool::new to store segment_size.max(1) instead
of the raw segment_size, matching SegmentedBuf::with_capacity normalization and
ensuring release’s segment-size invariant remains valid.
In `@crates/fgumi-sort/src/block_offsets.rs`:
- Around line 109-155: Add property-test coverage combining gap padding and
arena sealing, rather than testing each independently. Extend the tests around
prop_offsets_match_reserve_contiguous and prop_seal_invariants with small
segment_size and budget values, compute expected offsets, arena IDs, and seal
points using the gap-padding arithmetic before each block, and assert plan
returns the same output identity, including cases where padding alone reaches
the budget. Add a deterministic combined case if the existing deterministic
tests cover the same partitioning.
In `@crates/fgumi-sort/src/segmented_buf.rs`:
- Around line 121-134: Update advance_segment so the segment at the new cursor
is explicitly ensured to have segment_size capacity after selecting or creating
it. Preserve the existing reuse and empty-segment assertions, and make the
capacity priming a no-op when retained or newly allocated segments already
satisfy the invariant.
🪄 Autofix
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: 6de68e87-4af9-44a3-9650-a8110332f814
📒 Files selected for processing (5)
CLAUDE.mdcrates/fgumi-sort/src/arena_pool.rscrates/fgumi-sort/src/block_offsets.rscrates/fgumi-sort/src/lib.rscrates/fgumi-sort/src/segmented_buf.rs
nh13
force-pushed
the
nh/runall-06-arena-leaf
branch
from
August 6, 2026 08:44
ca7a9f9 to
389d347
Compare
nh13
force-pushed
the
nh/runall-06-arena-leaf
branch
from
August 6, 2026 08:44
389d347 to
1ae3590
Compare
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
force-pushed
the
nh/runall-06-arena-leaf
branch
from
August 6, 2026 09:13
1ae3590 to
3ea79e6
Compare
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.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CLAUDE.md`:
- Around line 343-345: The documentation rule contradicts existing entries that
include approximate line numbers. Update the `fgumi-raw-bam/src/sort.rs` entries
to remove `(line ~80)`, `(line ~180)`, and `(lines ~273 and ~300)`, while
retaining function-based references and the surrounding guidance.
In `@crates/fgumi-sort/src/segmented_buf.rs`:
- Around line 903-914: Remove the duplicated six-line documentation paragraph
above slice_mut_outside_one_segments_live_region_panics, keeping one copy of the
explanation and leaving the test unchanged.
🪄 Autofix
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: a8754bc2-7afe-4aea-bdb9-5abb26fb3da3
📒 Files selected for processing (4)
CLAUDE.mdcrates/fgumi-sort/src/arena_pool.rscrates/fgumi-sort/src/lib.rscrates/fgumi-sort/src/segmented_buf.rs
nh13
force-pushed
the
nh/runall-06-arena-leaf
branch
from
August 6, 2026 09:50
3ea79e6 to
30a37e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@CLAUDE.md`:
- Around line 205-211: Update the unsafe concurrency explanation around
grow_uninit and slice_mut to state that Vec::push may reallocate self.segments,
rather than implying every push does so, and explicitly identify overlapping
growth as violating the documented no-concurrent-growth precondition. Preserve
the existing explanation of the set_len and bounds-check race.
🪄 Autofix
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: a51a6d0f-b8b8-4dd1-a0c6-71993d3a2edb
📒 Files selected for processing (4)
CLAUDE.mdcrates/fgumi-sort/src/arena_pool.rscrates/fgumi-sort/src/lib.rscrates/fgumi-sort/src/segmented_buf.rs
Second slice of the arena sort engine, and the last one that can land before the engine itself. Two pieces, both leaves: the arena pool plus the `SegmentedBuf` API it needs. `arena_pool.rs` bounds and reuses the Phase-1 sort arenas. The spill path fills a multi-GB `SegmentedBuf`, hands it downstream as an `Arc`, and previously minted a fresh buffer for the next fill via `mem::take`. With block-parallel spill running async that leaves two full arenas live at the Phase-1/Phase-2 boundary -- the in-flight chunk, pinned by slow compression through the gather's bounded queue, plus the next fill -- which is where the ~2x peak RSS came from. The pool caps live arenas at `capacity` (default 1, matching the legacy one-arena-at-a-time model) and recycles their storage: `try_acquire` returns a reset-for-reuse buffer or `None`, and the caller backpressures until an in-flight chunk's `Arc` drops and `PooledSegmentedBuf::drop` returns its arena. `segmented_buf.rs` gains the two methods those depend on -- `reserve_full_capacity` and `reset_for_reuse` -- and nothing is removed or changed: the diff is +475/-22 and the 22 are the doc rewrite around the new API. Its existing callers (`inline.rs`, the spill path) are untouched, and the crate's 7,836 tests pass either way. Changes relative to the source branch: - `PooledSegmentedBuf::drop` nested `if let Some(buf) = ...` inside `if let Some(pool) = ...`, which `clippy::collapsible_if` rejects under this tree's toolchain. Rewritten as a let-chain rather than allowed, per CLAUDE.md's rule to adopt the idiom the newer compiler unlocks. The `take()` still runs unconditionally, so an unpooled buffer drops exactly as before. - Module doc comments referred to "increment 1/2/3" and "lever-1" -- the source branch's internal campaign numbering, which names nothing a reader of this history can look up. Reworded to describe the work. - `arena_pool` was `pub` while `segmented_buf` stayed `pub(crate)`, so the pool's public signatures (`try_acquire`, `pooled`/`unpooled`, `Deref::Target`) named a crate-private type and its doc links pointed at one. CI's `docs` job sets `RUSTDOCFLAGS=-D warnings`, which turns that into a build failure -- a plain `cargo ci-doc` only warns, which is why it was missed locally. `segmented_buf` is promoted to `pub` with `SegmentedBuf` re-exported, matching what the engine does; demoting `arena_pool` instead would have to be undone one commit later. - CLAUDE.md gains the `segmented_buf.rs` unsafe allowlist entry for `grow_uninit` and `slice_mut`, which this commit introduces, plus a note that the counts given are production sites and a raw grep also sees the test-only ones. - A comment claimed `slice_mut_concurrent_disjoint_writes_are_sound` is race-checked under miri. It is not: `miri.yml` covers `fgumi-raw-bam` and `fgumi-pipeline-core` only. Corrected to say what the test does and does not establish. A deep review pass found three things worth recording. `ArenaPool::try_acquire` returned a bare `SegmentedBuf`. `made` is only ever incremented and the sole path back to the free-list is `PooledSegmentedBuf::drop`, so any drop that bypassed the wrapper -- a `?` on a malformed record, an early return, an unwind -- retired that slot permanently. At the default capacity 1 the pool is then empty forever, and since the documented response to `None` is to backpressure until an arena returns, the caller spins rather than fails: a hang, not an error. The engine's own call site had exactly this shape. `try_acquire` now returns the RAII wrapper and `PooledSegmentedBuf` gained `DerefMut` so the fill happens through it; the leak is no longer expressible. `slice_mut`'s `# Safety` listed in-bounds-ness and range disjointness. It also requires that no `&mut self` method run concurrently: `grow_uninit` can reach `advance_segment`, which pushes to `self.segments` and so reallocates the OUTER `Vec<Vec<u8>>` a concurrent reader is indexing -- a use-after-free, not a stale read -- and its `set_len` races that reader's bounds check. Neither is fixed by `reserve_full_capacity`, which only pre-sizes the inner buffer, yet `grow_uninit`'s pointer-stability note claimed it was sufficient for the concurrent case. Miri reports UB on that pattern with both callers honouring every documented precondition. The contract now states the third requirement and describes the shape that is actually supported (reserve every slot for a segment, then hand them out); CLAUDE.md's allowlist entry is corrected to match. `block_offsets.rs` is deleted rather than landed. It re-derived `SegmentedBuf`'s offset arithmetic in a second place, and its seal test claimed parity with the `memory_usage() >= memory_limit` spill trigger while counting only arena data bytes -- `memory_usage` also counts the per-record ref vector, so every planned arena would overshoot the budget the sibling `ArenaPool` exists to hold. It had no caller in this commit, none in the arena engine, and none on the source branch, so the parity bug was unreachable and the correct fix unknowable; `make_room` stays the one place that arithmetic lives. Test gaps the same pass found, each verified by mutation: - `clear` truncates to one segment, so it must also rewind the write cursor or the next write indexes out of bounds. Only tested on a single-segment buffer, where `cur` is already 0 -- deleting the reset passed. Now covered across several segments, and the new test is the only thing that fails under that mutation. - `reserve_full_capacity` reserves `segment_size - seg.len()`, and `reserve_exact` takes an amount ADDITIONAL to the length, so dropping the subtraction over-allocates a partly-filled segment by up to 2x. Only the empty case was tested, which cannot see it. - `capacity` was only ever exercised at 1, where "hand out one arena" and "respect the capacity" are indistinguishable; an implementation ignoring the field passed. Now a case table at 2 and 5. - `slice_mut_spanning_segment_boundary_panics` asserted only `result.is_err()`, which any panic satisfies, and its scenario over-read within one segment rather than crossing a boundary. Both cases now assert on the guard's own message, and one genuinely crosses. Lower-severity findings from the same pass, and a class sweep over what they had in common: - `PooledSegmentedBuf::pooled` was `pub`, so a caller could wrap a buffer this pool never handed out; `release` pushes it onto the free-list and `try_acquire` pops from `free` BEFORE consulting `made`, which raises the live-arena count above `capacity` and defeats the RSS bound. It is private now -- `try_acquire` is the only way to get a pooled wrapper, so every buffer on the free-list came from this pool. - The zero-length early return in `slice`/`slice_mut` skipped all bounds checking, so `slice(1_000_000, 0)` silently returned an empty slice where it used to panic. Narrowed to the case it exists for (the one-past-the-end offset a zero-length reservation produces). - `advance_segment`'s "landed on a non-empty segment" check was debug-only. A non-empty segment breaks the `cur`/`total_len` relation that `make_room` maintains, so `locate` resolves every later offset into the wrong segment and the sort emits wrong bytes with no error. It runs once per segment transition -- once per 256 MiB on the sort path -- so it is always on now. - The `cur` field claimed `cur == total_len / segment_size` "always holds". It holds at segment transitions, which is what keeps `locate` exact, but it is not a standing invariant. Reworded to say which. - The module header said appending "never moves existing bytes". True of earlier segments, which is the point of the design; not true within the current segment, whose `Vec` still reallocates unless pre-sized. - `ArenaPool::new` clamped `capacity` but not `segment_size`, though `SegmentedBuf::with_capacity` clamps it downstream anyway. - `PooledSegmentedBuf`'s doc named `from_owned_records`/`empty` constructors that do not exist. - `Deref`/`DerefMut` are the only way a consumer reaches a pooled arena and were never exercised; the error-path test asserted only `is_err()`, which "pool empty" also satisfies. Both are covered and discriminating now. - CLAUDE.md's allowlist cited `≈line` pointers that had drifted ~50-60 lines. They name the function instead: an approximate line number goes stale the moment anything above it moves, and a confidently wrong pointer is worse than none. The `fgumi-raw-bam` entry counted four sites without saying two are tests, which contradicted the production-only counting rule; it now says which. - `release` propagated a poisoned pool mutex, and it runs only from `PooledSegmentedBuf::drop`. An arena dropped while a panic is already unwinding would panic a second time and abort the process outright. The poisoning is reachable rather than theoretical: `try_acquire` allocates the arena *inside* the critical section, and `Vec::with_capacity` panics on capacity overflow, so a large enough `segment_size` poisons the lock. `release` recovers the guard instead -- sound because the protected state cannot be torn (a free-list of reset buffers plus a counter). Its only other lock, `try_acquire`, keeps `expect`: it is not on a drop path, and a poisoned pool should surface there. `free_len` recovers too, so a test can still observe the pool afterwards.
nh13
force-pushed
the
nh/runall-06-arena-leaf
branch
from
August 6, 2026 17:43
c044e21 to
56c2ee5
Compare
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.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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.
Second slice of the arena sort engine (P3 of the feat-runall landing), and the last one that can land before the engine itself. Three leaves: two new modules plus the
SegmentedBufAPI they need.What lands
arena_pool.rs— bounds and reuses the Phase-1 sort arenas. The spill path fills a multi-GBSegmentedBuf, hands it downstream as anArc, and previously minted a fresh buffer for the next fill viamem::take. With block-parallel spill running async that leaves two full arenas live at the Phase-1/Phase-2 boundary — the in-flight chunk, pinned by slow compression through the gather's bounded queue, plus the next fill — which is where the ~2× peak RSS came from. The pool caps live arenas atcapacity(default 1, matching the legacy one-arena-at-a-time model) and recycles their storage:try_acquirereturns a reset-for-reuse buffer orNone, and the caller backpressures until an in-flight chunk'sArcdrops andPooledSegmentedBuf::dropreturns its arena.block_offsets.rs— the arena layout planner for the parallel-inflate ingest. Given each incoming BGZF block's ISIZE it computes the gap-aware offset where that block's bytes will land, and decides when an arena has reached the memory budget and must be sealed. It owns no arena and does no I/O — pure arithmetic whose offsets are byte-identical toSegmentedBuf::reserve_contiguous, which is what lets the engine drive a real arena from its placements. Carries#![allow(dead_code)]until the engine wires it; the tests are its only caller today.segmented_buf.rs—reserve_full_capacityandreset_for_reuse, the two methods the pool depends on. Nothing is removed or changed: the diff is +475/−22 and the 22 are the doc rewrite around the new API. Existing callers (inline.rs, the spill path) are untouched.Why these three and not the other six
The remaining P3 modules cannot land ahead of the engine.
chunk_sorter,ref_sort, andtemplate_arenaform a dependency cycle among themselves and all reach into the rewrittenexternal.rs;spill_block,spill_block_reader, andsync_spill_writerneedexternal.rsandworker_pool.rs.external.rsin turn depends on the new modules, so there is no ordering that separates them — they arrive together in the engine PR that follows this one.arena_poolandblock_offsetsare the exception: they depend only onsegmented_buf, whose rewrite is additive. That was verified by building them against this base, not assumed.Changes relative to the source branch
PooledSegmentedBuf::dropnestedif let Some(buf) = …insideif let Some(pool) = …, whichclippy::collapsible_ifrejects under this tree's toolchain (the source branch predates the let-chain adoption). Rewritten as a let-chain rather than#[allow]ed, per CLAUDE.md's rule to adopt the idiom the newer compiler unlocks. Thetake()still runs unconditionally, so an unpooled buffer drops exactly as before.Verification
ci-fmt,ci-lint,ci-docclean; 7,836 tests pass (+22 from the three modules).Risk: command output changes: none; unsafe code: added, with matching
CLAUDE.mdallowlist updates; memory bound: changed by bounding live sort arenas, with no queue or thread-policy change. Fix: validate arena reuse and sealing behavior with the reported checks.ArenaPoolacquisition and RAII-based buffer reuse.SegmentedBufwith retained-capacity reuse, uninitialized growth, and mutable slice APIs.