Repository navigation
perf(sort): borrow record bytes from the decompressed block on ingest - #437
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat-runall #437 +/- ##
==============================================
Coverage ? 93.39%
==============================================
Files ? 139
Lines ? 54817
Branches ? 0
==============================================
Hits ? 51199
Misses ? 3618
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
crates/fgumi-sort/src/read_ahead.rs (1)
660-814: 🏗️ Heavy liftAdd a
proptestparity/property test for borrowed ingest.Given boundary-heavy logic (prefix/body straddles), add randomized property checks (record sizes/content/block sizes) alongside the fixed examples.
As per coding guidelines:
Use proptest for property-based testing in Rust test files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fgumi-sort/src/read_ahead.rs` around lines 660 - 814, Add a `proptest`-based property test that generates randomized record bodies and block sizes to complement the fixed example tests. Create a new test function (similar to test_next_record_borrowed_parity_with_read_record) that uses proptest strategies to randomly generate various record sizes, content patterns, and block lengths, then verify that the borrowed ingest path produces byte-identical results to either the owned read_record path or the input records. This should test a wider range of boundary conditions than the current fixed test cases like test_next_record_borrowed_prefix_straddle and test_next_record_borrowed_body_exact_boundary_and_straddle.Source: Coding guidelines
🤖 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/read_ahead.rs`:
- Around line 733-742: The test function
test_next_record_borrowed_matches_input_across_block_sizes uses a for loop to
iterate over different block_len values instead of using rstest for
parameterization. Refactor this test by adding the #[rstest] attribute macro and
converting the for loop into a parameterized block_len parameter using
#[values(...)] with the array of test values (1usize, 2, 3, 4, 5, 6, 7, 8, 16,
64, 1024, 65_535). Remove the for loop and make block_len a direct parameter of
the test function. Apply the same refactoring pattern to the other test
mentioned at lines 772-794.
---
Nitpick comments:
In `@crates/fgumi-sort/src/read_ahead.rs`:
- Around line 660-814: Add a `proptest`-based property test that generates
randomized record bodies and block sizes to complement the fixed example tests.
Create a new test function (similar to
test_next_record_borrowed_parity_with_read_record) that uses proptest strategies
to randomly generate various record sizes, content patterns, and block lengths,
then verify that the borrowed ingest path produces byte-identical results to
either the owned read_record path or the input records. This should test a wider
range of boundary conditions than the current fixed test cases like
test_next_record_borrowed_prefix_straddle and
test_next_record_borrowed_body_exact_boundary_and_straddle.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08b6d256-f867-47dc-8709-a63eb46d4df1
📒 Files selected for processing (4)
crates/fgumi-raw-bam/src/lib.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-sort/src/external.rscrates/fgumi-sort/src/read_ahead.rs
On the pooled sort ingest path each record's bytes were copied twice after decompression: once from the decompressed block into a `RawRecord` (via `read_exact`) and again from the `RawRecord` into the sort arena. The first copy is removable when the record body lies wholly within the current decompressed block, which is the common case. Add `PooledInputStream::next_record_borrowed`, a lending reader that returns a slice borrowed directly out of `current_buf` on the fast path, falling back to a reusable scratch buffer only when the record body or its 4-byte length prefix straddles a decompressed-block boundary. `RecordSource` grows a matching `next_record_borrowed` (the `ReadAhead` thread variant lends the owned record it just received). The coordinate and template-coordinate ingest loops now consume borrowed slices and push them straight into the buffer, dropping the intermediate `RawRecord` copy (copy amplification 2 -> 1; input-read memmove was ~8% of sort CPU). The keyed/queryname path keeps owned records, which it must retain. Byte-identical output verified by an A/B over a 8.2M-record BAM (sorted record streams compare equal) and by a parity unit test against the owned `read_raw_record` path. Adds dedicated tests for records and length prefixes straddling block boundaries across many block sizes (down to 1 byte). Closes #436
nh13
force-pushed
the
feat-runall-36-sort-ingest-borrow
branch
from
June 18, 2026 16:37
41bc769 to
6a241a7
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
nh13
added a commit
that referenced
this pull request
Jun 18, 2026
…#437) On the pooled sort ingest path each record's bytes were copied twice after decompression: once from the decompressed block into a `RawRecord` (via `read_exact`) and again from the `RawRecord` into the sort arena. The first copy is removable when the record body lies wholly within the current decompressed block, which is the common case. Add `PooledInputStream::next_record_borrowed`, a lending reader that returns a slice borrowed directly out of `current_buf` on the fast path, falling back to a reusable scratch buffer only when the record body or its 4-byte length prefix straddles a decompressed-block boundary. `RecordSource` grows a matching `next_record_borrowed` (the `ReadAhead` thread variant lends the owned record it just received). The coordinate and template-coordinate ingest loops now consume borrowed slices and push them straight into the buffer, dropping the intermediate `RawRecord` copy (copy amplification 2 -> 1; input-read memmove was ~8% of sort CPU). The keyed/queryname path keeps owned records, which it must retain. Byte-identical output verified by an A/B over a 8.2M-record BAM (sorted record streams compare equal) and by a parity unit test against the owned `read_raw_record` path. Adds dedicated tests for records and length prefixes straddling block boundaries across many block sizes (down to 1 byte). Closes #436
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.
Summary
On the pooled sort ingest path each record's bytes were copied twice after decompression:
This removes copy #1 on the common path. A new
PooledInputStream::next_record_borrowedreturns a slice borrowed directly out ofcurrent_bufwhen the record body lies wholly within the current decompressed block, falling back to a reusable scratch buffer only when the record body or its 4-byte length prefix straddles a block boundary.RecordSourcegrows a matchingnext_record_borrowed; the coordinate and template-coordinate ingest loops now consume borrowed slices and push them straight into the buffer (copy amplification 2 → 1). The keyed/queryname path keeps ownedRawRecords, which it must retain forVec<(K, RawRecord)>.Input-read
memmovemeasured ~8% of sort CPU on a Time Profiler offgumi sort; this drops one of the two record-body copies (PooledInputStreampath only — it does not touch the ~63% that is BGZF de/compression).Correctness
This is a core ingest-path refactor, so correctness was prioritized over speed:
-m 8GiB,--threads 8): the sorted record streams compare equal (samtools view | cmp). The only.bam-level difference is BGZF block boundaries, which are already non-deterministic under multithreaded compression onfeat-runallitself.read_raw_recordpath over identically-chunked streams.unsafe— the fast path is a slice borrow and the slow path is aread_exactinto reused scratch.Validation
cargo build --features compare,simulate,profile-adjacencycargo ci-lint(clippy pedantic,-D warnings) — cleancargo ci-fmt— cleancargo nextest -p fgumi-sort— 476 passed (6 newnext_record_borrowedtests)cargo nextest --features compare,simulate,profile-adjacency -E 'test(sort) or test(runall) or test(streaming)'— 296 passedCloses #436