Repository navigation
perf(raw-bam): follow-up speedups + SIMD nibble2base - #282
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #282 +/- ##
=======================================
Coverage 90.02% 90.03%
=======================================
Files 122 122
Lines 61791 61791
=======================================
+ Hits 55630 55631 +1
+ Misses 6161 6160 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 46 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis pull request optimizes and extends the raw-byte BAM API in 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/fgumi-raw-bam/src/sequence.rs (1)
223-228: Fail fast on undersizeddst.This is a public API, but the length contract is only
debug_assert!ed. In release builds, misuse turns into a later bounds panic instead of failing at the call boundary. Prefer an unconditionalassert!with a clear message, or return aResult.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/sequence.rs` around lines 223 - 228, The public API pack_sequence_into_slice currently only uses debug_assert! to check dst length, which fails to enforce the contract in release builds; replace the debug_assert! with an unconditional assert! (e.g. assert!(dst.len() >= packed_len, "pack_sequence_into_slice: destination slice too small; need {} bytes", packed_len)) so callers immediately get a clear panic on misuse, or alternatively change the function to return Result<(), Error> and return Err when dst.len() < packed_len; apply this change inside pack_sequence_into_slice where packed_len is computed and reference dst and bases accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fgumi-raw-bam/src/cigar.rs`:
- Around line 56-64: The right-shift uses (op_type << 1) which can be >=32 and
panic in debug builds; add an explicit guard at the start of both consumes_ref
and consumes_query to return false for out-of-range op_type values (e.g., if
op_type >= 16 { return false }) before performing the (BAM_CIGAR_TYPE >>
(op_type << 1)) bit operations so the shift is always <32 and behavior for
unknown op codes remains false.
In `@crates/fgumi-raw-bam/src/sequence.rs`:
- Around line 769-785: The test
test_extract_sequence_simd_matches_scalar_over_lengths currently only
round-trips pack_sequence_into -> extract_sequence and doesn't verify SIMD vs
scalar; change it to compute an explicit expected sequence by decoding the same
packed bytes with the scalar decoder (call get_base on each packed nibble and
map via BAM_BASE_TO_ASCII) over the full alphabet (=ACMGRSVTWYHKDBN) for each
index, then assert extract_sequence(&bam) == expected; keep using
pack_sequence_into, seq_offset, make_bam_bytes and the same length cases
(including l >= 32) so the SIMD path is exercised while verifying the scalar
get_base/BAM_BASE_TO_ASCII result against extract_sequence.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/src/sequence.rs`:
- Around line 223-228: The public API pack_sequence_into_slice currently only
uses debug_assert! to check dst length, which fails to enforce the contract in
release builds; replace the debug_assert! with an unconditional assert! (e.g.
assert!(dst.len() >= packed_len, "pack_sequence_into_slice: destination slice
too small; need {} bytes", packed_len)) so callers immediately get a clear panic
on misuse, or alternatively change the function to return Result<(), Error> and
return Err when dst.len() < packed_len; apply this change inside
pack_sequence_into_slice where packed_len is computed and reference dst and
bases accordingly.
🪄 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: ea2009b3-6e28-4ce5-9b6a-2a5e14601ab4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlbenches/raw_bam_accessors.rscrates/fgumi-raw-bam/Cargo.tomlcrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/fields.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/src/sequence.rscrates/fgumi-raw-bam/src/tags.rs
40211c0 to
40ffceb
Compare
cc7a203 to
f27ff46
Compare
Six optimizations on top of the raw-byte view/editor API: - tag_value_size: #[inline(always)] so the inner dispatch of the hot find_tag_position loop inlines the table-lookup + match. - find_tag_position: compare the 2-byte tag identifier as a single u16 instead of a slice reference. - aux_data_offset_from_record: single u64 load over offsets 12..20 to fetch n_cigar_op and l_seq in one go (flag at 14..15 masked off). - set_sequence_and_qualities: shift aux in place with copy_within and write packed seq + qual directly into the record buffer, eliminating the intermediate Vec. Bench: set_sequence_and_qualities_grow 300 ns -> 125 ns (-58%); vs-RecordBuf speedup 6x -> 13.8x. - extract_sequence: SIMD nibble2base via wide::u8x16::swizzle_relaxed (portable pshufb). 16 packed bytes = 32 bases per iteration. 150 bp ~3.8x, 300 bp ~6.0x vs scalar. Scalar path retained for l_seq < 32. - consumes_query / consumes_ref: single shift+mask against BAM_CIGAR_TYPE = 0x3C1A7 (htslib's sam.h). Adds RawRecordView::cigar_lengths() for a dual ref_len + query_len walk. Also bumps workspace wide to 1.3 and adds bytemuck/wide to fgumi-raw-bam. Refs #272
f27ff46 to
01baf28
Compare
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
… remaining 48-commit work (#296) The final PR in the stacked series closes the full delta against the 48-commit backup. In addition to the original scope (vendored bam_codec deletion + simulate migration), this amends to include: - Missing new files: sort/keys.rs, 4 raw-bam bench files - compare/bams.rs RawRecord migration - codec_caller.rs cascade simplification - group/filter/correct/extract/clip/bam_io.rs simplifications - overlapping.rs raw-only API - Test suite updates for test_compare_bams, test_group_command, test_e2e_regression - Doc touch-ups in best-practices/performance-tuning Verified: tree matches backup except for 6 main-only files (prefetch_reader, os_hints, test_async_reader, template_coordinate, and two benches) that came from main PRs #258/#261/#265/#270/#282 merged after our branch point. Part 8 of 8 in the series closing #272. Stacks on #295.
Summary
Part 5 of 5 in the stacked series closing #272. Stacks on #281.
Six perf optimizations on top of the core API, grouped into three commits:
f0b8b32— small tag-lookup/header-decode winstag_value_sizemarked#[inline(always)](hot insidefind_tag_position)find_tag_positioncompares tags asu16instead of byte slicesaux_data_offset_from_recorddoes a single u64 load over offsets 12..20bea6016—set_sequence_and_qualitiessplices in placeVec::copy_within+pack_sequence_into_slicereplace intermediateVecallocationset_sequence_and_qualities_grow300 ns → 125 ns (−58%); vs-RecordBuf speedup 6× → 13.8×df1089c+dd8b4fb— SIMD nibble2base viawide::u8x16::swizzle_relaxed(portablepshufb)simd.cnibble2basesequence_vec150 bp: 3.8×, 300 bp: 6.0×0955cfa— branchlessBAM_CIGAR_TYPE = 0x3C1A7bitmask (matches htslib)consumes_query/consumes_refbecome single shift+maskRawRecordView::cigar_lengths()for dual-return walk (ref_len + query_len in one iteration)Benchmark summary
See the comparison table in PR description for the raw-byte-vs-
RecordBufspeedups after all perf work: 14×–922× range, 13.8× on the previously-weakestset_sequence_and_qualities.Test plan
cargo ci-test— all tests passcargo bench --bench raw_bam_accessors— SIMD numbers confirmedcargo bench --bench raw_bam_vs_recordbuf— post-refactor speedups measuredStack
Closes #272 (when the full stack lands). Supersedes #277.