Repository navigation
perf(decode): fold UMI-position cache into the group-key aux scan - #976
Conversation
The decode step scanned each record's aux block twice: once in `compute_group_key_from_raw` (via `extract_aux_string_tags`) for the RG/cell/MC group key, then again in `cache_umi_position` (via `find_string_tag_position`) for the UMI value position (#334). The single-pass extractor already accepts a `umi_tag` and returns the UMI position, so the second walk is redundant on the common `KeyMode::Full` path. Add `compute_group_key_and_umi_from_raw` / `key_and_umi_for_mode`, which pass the UMI tag into the same `extract_aux_string_tags` call that builds the key and return the record-relative `(offset, len)`. The BAM and SAM decode paths use these via a new `apply_cached_umi` helper and fall back to a standalone `cache_umi_position` scan only when the key was built without reading aux data (`KeyMode::None`/`NameHashOnly`, or a name-only key path). Because the UMI is now resolved by the same `extract_aux_string_tags` pass as RG/cell/MC, its duplicate/mistyped-tag handling matches the key's own tag resolution rather than the standalone scan's. For spec-legal records (each aux tag at most once, SAM §1.5) the folded capture yields the identical position the standalone scan produced; the two differ only on malformed duplicate/type-shadowed tags — the same deliberate, already-documented divergence the `MC` tag carries via `validate_mc_tag`. Output is byte-identical on well-formed input: `fgumi compare bams` reports 0 content diffs for both group and dedup over a 60M-record benchmark BAM, and the SAM/BAM parity and group/dedup MI-determinism tests pass unchanged. Removes the per-record `cache_umi_position` aux walk (~2% of single-thread CPU on group, ~1.4% on dedup, Graviton4) from the shared decode path, which also serves consensus and `runall`'s single decode.
|
Note Reviews pausedUse 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: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe grouping API now returns an optional record-relative UMI position with each group key. Decode and SAM parsing paths reuse that position and scan only when capture is unavailable. ChangesInline UMI Capture
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant Parser
participant key_and_umi_for_mode
participant apply_cached_umi
participant DecodedRecord
Parser->>key_and_umi_for_mode: compute GroupKey and optional UMI position
key_and_umi_for_mode-->>Parser: return GroupKey and UMI position
Parser->>apply_cached_umi: apply position or fallback scan
apply_cached_umi->>DecodedRecord: cache UMI bytes
Merge Risk: ⚪ Minimal · up to The optimized UMI caching preserves the record-byte offsets consumed by parsing, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #976 +/- ##
==========================================
- Coverage 96.12% 96.09% -0.03%
==========================================
Files 293 293
Lines 146660 146751 +91
==========================================
+ Hits 140975 141022 +47
- Misses 5685 5729 +44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
What
The decode step scanned each record's aux block twice on the group/dedup/consensus hot path: once in
compute_group_key_from_raw(viaextract_aux_string_tags) for the RG/cell/MC group key, then again incache_umi_position(viafind_string_tag_position) for the UMI value position (#334). The single-pass extractor already accepts aumi_tagand returns the UMI position, so the second walk is redundant.This folds the UMI-position capture into the same aux scan that builds the key:
compute_group_key_and_umi_from_raw/key_and_umi_for_modepass the UMI tag into the oneextract_aux_string_tagscall and return the record-relative(offset, len).DecodeRecords,DecodeFromRecords) and SAM (ParseSamChunk) decode paths use these via a newapply_cached_umihelper, and fall back to the standalonecache_umi_positionscan only when the key was built without reading aux data (KeyMode::None/NameHashOnly, or a name-only key path).compute_group_key_from_raw/key_for_modeare kept as thin wrappers.Because the fused pipeline decodes once, this also trims
runall's single decode, not just the standalone commands.Tag-resolution semantics (byte-identity scope)
The UMI is now resolved by the same
extract_aux_string_tagspass as RG/cell/MC, so the cached UMI is consistent with the key's own tag resolution. For spec-legal records (each aux tag present at most once, SAM §1.5) this yields the identical position the standalonefind_string_tag_positionscan produced. The two deliberately differ only on malformed duplicate/type-shadowed tags — the same already-accepted divergence theMCtag carries viavalidate_mc_tag(src/lib/grouper.rs):extract_aux_string_tagsskips a non-Zentry and takes a laterZcopy, whereasfind_tag_positionresolves the first id match and rejects it if non-Z. The relaxation makes the cached value agree with the value the grouping key actually uses.Verification
fgumi compare bamsreportsContent diffs: 0/ IDENTICAL for both group and dedup over a real 60M-record benchmark BAM.runallstaged-vs-fused tests pass unchanged; new parity tests assert the folded capture equals the standalone scan on the primary, single-end, andtc-stamped secondary/supplementary paths, plus theapply_cached_umifallback branch.cargo clippy --all-targets --all-features -- -D warnings -W clippy::pedantic,cargo fmt, and rustdoc all clean.Measurement
At
t=1on a 60M-read, 1%-error grouped BAM (Graviton4), CPU-seconds, 2 reps:The removed work is the per-record
cache_umi_positionaux walk; it is part of the shared decode path, so simplex/duplex/codec andrunall's single decode benefit as well.Risk: output changes — none for well-formed input, pinned by parity tests and existing tag-resolution behavior; unsafe changes — none, so no
CLAUDE.mdallowlist update is needed; memory bounds, queue capacity, and thread/backpressure policy changes — none.Fix: capture the UMI position during the existing grouping-key auxiliary-tag scan and reuse it during BAM and SAM decoding.
key_and_umi_for_mode.tcsecondary/supplementary records.