Repository navigation
perf(sort): narrow radix passes, reuse the stored queryname NUL, and borrow record bytes on ingest - #606
Merged
Conversation
|
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 (9)
WalkthroughThe PR adds borrow-in-place BAM access, routes coordinate and template ingestion through borrowed bytes, bounds radix passes using mapped keys, hardens queryname extraction, and adds exports, tests, and benchmarks. ChangesBorrowed ingest and sorting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #606 +/- ##
==========================================
+ Coverage 93.43% 93.45% +0.02%
==========================================
Files 175 175
Lines 104981 105405 +424
==========================================
+ Hits 98089 98511 +422
- Misses 6892 6894 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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: 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 `@crates/fgumi-sort/src/inline.rs`:
- Around line 1658-1690: Update bytes_needed in radix_sort_record_refs_with_max
so it increases by one when max_key fills all bits of its currently computed
byte width, preventing mapped all-0xFF keys from tying with the u64::MAX
sentinel. Add a regression covering 0xFFFF-class mapped keys mixed with unmapped
records and verify the sentinel ordering is correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d37c4da-e0ab-4354-b06c-651b786137e2
📒 Files selected for processing (9)
CLAUDE.mdbenches/core_functions.rscrates/fgumi-raw-bam/src/lib.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-sort/src/external.rscrates/fgumi-sort/src/inline.rscrates/fgumi-sort/src/keys.rscrates/fgumi-sort/src/lib.rscrates/fgumi-sort/src/read_ahead.rs
…stores `extract_queryname_key` stripped the trailing NUL that BAM stores and then re-appended one into a freshly allocated `Vec` for every record. BAM already stores the read name NUL-terminated and `l_read_name` counts the terminator, so `bam[32..32 + l_read_name]` is directly usable as the key bytes. The fast path only applies when the declared name is in bounds *and* the byte it ends on is genuinely a NUL. That second condition is a soundness requirement, not a nicety: `RawQuerynameKey::cmp` passes `name.as_ptr()` to `natural_compare_nul`, which scans until it finds a terminator, so a key built from a malformed record whose `l_read_name` does not land on one would read past the end of its own allocation. Records failing either condition fall back to the previous strip-and-re-append path, which terminates unconditionally. Adds an rstest table that checks the extracted key against the pre-optimization implementation as an independent oracle, covering well-formed, empty-name, truncated, missing-terminator, non-NUL-terminated, and zero-length records, and asserts the null-termination invariant on each.
…ed sentinel
Coordinate keys pack `(tid << 34) | ((pos + 1) << 1) | reverse`, so a mapped key
occupies only ~5-6 bytes. Unmapped reads, however, carry `u64::MAX` as their
sort key, which dragged `bytes_needed` to the full 8 and cost three wasted LSD
radix passes over every record. Coordinate-sorted BAMs essentially always have
an unmapped tail, so this was paid on real input almost without exception.
Sizing the passes from the largest *non-sentinel* key fixes it, and needs no
contract from the caller: `u64::MAX` is the largest `u64` and truncates to the
all-`0xFF` maximum at every radix width, so unmapped records still sort to the
tail and stay stable among themselves no matter how many passes run. Both
scanning entry points derive their bound this way, and `par_sort_into_chunks`
derives one bound for the whole buffer and shares it across chunks.
A derived bound of 0 does not imply the keys are all equal, so the sort is never
skipped outright: a zero key can coexist with sentinels, and those two do not
compare equal. `PackedCoordinateKey::new` packs `tid = 0, pos = -1,
reverse = false` to exactly 0, so a malformed record makes this reachable, and
skipping would leave the sentinels ordered ahead of it. A single pass separates
them and is a stable no-op when the keys genuinely are identical.
Measured on an otherwise idle c6a.4xlarge over 25 contigs with a 5% unmapped
tail, criterion 100 samples (confidence intervals within +/-0.5%):
records before after speedup
1M 23.6 M/s 29.0 M/s 1.23x
8M 21.0 M/s 25.0 M/s 1.19x
An earlier revision also tracked the bound incrementally as records were pushed,
letting the sort skip the scan entirely. That was measured at a further 1.04x
(30.2 and 26.0 M/s respectively) -- roughly half a percent of total sort time,
since the radix is about a seventh of it -- and is not kept: it cost a running
field on `RecordBuffer`, three reset sites, an ordering hazard where the reset
had to precede a macro that returns from inside itself, and a public entry point
whose unchecked precondition silently mis-sorts when violated. The scan is three
lines and carries no invariant.
Tests cover sentinel handling through both scanning entry points in serial and
parallel, zero keys interleaved with sentinels above the radix threshold,
`par_sort_into_chunks` on both its single- and multi-threaded drain paths, a
`RecordBuffer` end-to-end mix of mapped and unmapped records, and an
output-identity check that a narrowed sort is byte-for-byte identical to a
full-width one including the stable order among equal keys. The added criterion
benchmark separates pass-count reduction from scan elimination.
Pooled sort ingest copied each record's bytes twice after decompression: once out of the decompressed block into a `RawRecord`, then again from that `RawRecord` into the sort arena. The first copy is avoidable whenever the record body lies wholly within the current block, which is the common case. Adds `PooledInputStream::next_record_borrowed`, a lending reader that returns a slice borrowed straight out of `current_buf`, falling back to a reusable scratch buffer only when the body or its 4-byte length prefix straddles a block boundary. The coordinate and template-coordinate ingest loops consume borrowed slices and push them into the buffer directly, taking copy amplification from two to one. The keyed/queryname path keeps owned records, which it requires. `RecordSource` grows a matching `next_record_borrowed`. Both non-pooled variants already yield owned records and so have nothing shared to borrow from: each stores the record it just took and lends a slice into it. This includes the `Stream` variant, which is not present upstream -- it gains a held slot alongside its error slot, and its producer-error handling is shared with the `Iterator` impl so a failure reaches `take_error()` identically on both paths. That matters because the ingest loops treat `Ok(None)` as end-of-input, so an error swallowed there would silently truncate a sort rather than fail it. Tests cover records and length prefixes straddling block boundaries down to 1-byte blocks, parity against the owned `read_raw_record` path, a proptest over randomized bodies and block sizes, and -- for the `Stream` variant -- agreement with owned iteration plus propagation of a mid-stream producer error.
nh13
force-pushed
the
nh/perf-sort-radix-key-borrow
branch
from
July 21, 2026 09:03
eba0243 to
f8a371b
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Merged
This was referenced Aug 7, 2026
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.
Three independent throughput improvements to the sort engine, ported from
feat-runalland re-verified againstmain. Output is byte-identical in all three cases; the only behavior changes are bug fixes described below.Suggested reading order is commit by commit — they touch different files and are independently reviewable.
perf(sort): build the natural queryname key from the NUL BAM already storesextract_queryname_keystripped the trailing NUL that BAM stores and re-appended one into a freshVecper record. BAM already stores the name NUL-terminated andl_read_namecounts the terminator, so the stored bytes are directly usable.The fast path applies only when the declared name is in bounds and the byte it ends on is genuinely a NUL. That second condition is a soundness requirement rather than a nicety:
RawQuerynameKey::cmppassesname.as_ptr()tonatural_compare_nul, which scans until it finds a terminator, so a key built from a record whosel_read_namedoes not land on one would read past the end of its own allocation. Anything failing either condition falls back to the previous strip-and-re-append path, which terminates unconditionally.Tested with an rstest table checking the new extractor against the pre-optimization implementation as an independent oracle, across well-formed, empty-name, truncated, missing-terminator, non-NUL-terminated, and zero-length records.
perf(sort): size radix passes from the max mapped key, not the unmapped sentinelMapped coordinate keys occupy ~5-6 bytes, but unmapped reads carry
u64::MAX, which draggedbytes_neededto the full 8 and cost three wasted LSD passes over every record. Coordinate BAMs essentially always carry an unmapped tail, so this was paid on real input almost without exception.Deriving the bound from the largest non-sentinel key needs no contract from the caller:
u64::MAXis the largestu64and truncates to the all-0xFFmaximum at every radix width, so unmapped records still sort to the tail and stay stable among themselves regardless of pass count.Measured on an otherwise idle
c6a.4xlargeover 25 contigs with a 5% unmapped tail, criterion 100 samples, confidence intervals within ±0.5%:Radix is roughly a seventh of sort CPU, so this is a low-single-digit percentage of total sort time — worth having, but not a headline number.
An earlier revision of this change also tracked the bound incrementally across pushes so the sort could skip the scan entirely. That measured at a further 1.04x — about half a percent of total sort time — and is deliberately not kept: it cost a running field on
RecordBuffer, three reset sites, an ordering hazard where the reset had to precede a macro that returns from inside itself, and a public entry point whose unchecked precondition silently mis-sorts when violated. The scan is three lines and carries no invariant.Bug fix included: a derived bound of 0 does not imply the keys are all equal, so the sort is never skipped outright. A zero key can coexist with sentinels and the two do not compare equal —
PackedCoordinateKey::newpackstid = 0, pos = -1, reverse = falseto exactly 0, so a malformed record makes this reachable, and skipping would leave the sentinels ordered ahead of it. A single pass separates them and is a stable no-op when the keys genuinely are identical. Covered by a regression test with zero keys interleaved with sentinels above the radix threshold.perf(sort): borrow record bytes from the decompressed block on ingestPooled ingest copied each record twice after decompression: block into a
RawRecord, thenRawRecordinto the sort arena.PooledInputStream::next_record_borrowedlends a slice straight out of the current decompressed block, falling back to a reusable scratch buffer only when the body or its 4-byte length prefix straddles a block boundary. Copy amplification goes from two to one on the coordinate and template-coordinate paths. The keyed/queryname path keeps owned records, which it requires.RecordSourcegrows a matchingnext_record_borrowed. Both non-pooled variants already yield owned records and have nothing shared to borrow from, so each stores the record it just took and lends into it. That includes theStreamvariant, which does not exist upstream — its producer-error handling is shared with theIteratorimpl so a failure reachestake_error()identically on both paths. This matters because the ingest loops treatOk(None)as end-of-input, so an error swallowed there would silently truncate a sort rather than fail it.Tested with records and length prefixes straddling block boundaries down to 1-byte blocks, parity against the owned
read_raw_recordpath, a proptest over randomized bodies and block sizes, and per-variant agreement plus mid-stream producer-error propagation.Verification
cargo ci-test(5573 tests),cargo ci-fmt,cargo ci-lint, andRUSTDOCFLAGS="-D warnings" cargo ci-docall pass. Patch coverage is 100% of changed lines. Each new test was checked for non-vacuity by breaking the corresponding fix and confirming the test fails.Summary by CodeRabbit