Repository navigation
perf(zipper): probe tag membership with a 256×256 bitset - #830
Conversation
|
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: Pro Run ID: 📒 Files selected for processing (1)
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. WalkthroughRaw SAM tag filtering now uses cached two-byte lookups instead of repeated UTF-8 conversions and string searches. ChangesRaw SAM tag lookup caching
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This localized performance change preserves the public API and documented behavior, with comprehensive tests and real-data parity validation; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
The per-record tag-copy loop in merge_raw probed three HashSet<String> sets (remove/reverse/revcomp) once per aux tag per mapped read, each lookup paying a SipHash of the tag name plus a UTF-8 conversion of the two raw tag bytes. Replace the hot-path probe with a direct bit test on the two tag bytes via a new TagBitset (256x256 bit table), built once per run into a ZipperTags struct and reused for every template. Measured on an M3 Ultra (aarch64): the bitset probe is ~27x faster per tag than the HashSet<String> lookup and removes ~23% of the per-record tag-copy loop on a consensus-config zipper (--tags-to-reverse Consensus --tags-to-revcomp Consensus), where the loop is otherwise dominated by remove_tag+append over the ~3.5 KB consensus aux. The bitset also decisively beats a HashSet<[u8;2]>, which still hashes. The public merge_raw(&TagInfo) API is unchanged: it now builds the lookups and delegates to an internal merge_raw_with. Only two-byte tag names are representable (all SAM tags are two bytes); longer/shorter filter strings are dropped at construction, matching the prior len() == 2 guard.
db22c10 to
bc5b9b7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #830 +/- ##
=======================================
Coverage 94.40% 94.41%
=======================================
Files 186 186
Lines 114369 114458 +89
=======================================
+ Hits 107974 108061 +87
- Misses 6395 6397 +2 ☔ 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
Summary
The per-record tag-copy loop in
merge_raw(zipper) probed threeHashSet<String>sets (remove/reverse/revcomp) once per aux tag per mapped read — each lookup paying a SipHash of the tag name plus a UTF-8 conversion of the two raw tag bytes. This replaces the hot-path probe with a direct bit test on the two tag bytes via a new privateTagBitset(256×256 bit table), built once per run into aZipperTagsstruct and reused for every template.Approach
TagBitset— a 256×256 bit table (1024u64words) keyed by the two raw tag bytes;containsis a single bit test, no hash and no UTF-8 conversion. Only two-byte tag names are representable (all SAM tags are two bytes), so longer/shorter filter strings are dropped at construction — mirroring the priortag_str.len() == 2guard.ZipperTags— precomputes the three bitsets plus the two-byteremove_list(for the Step-2 tag removal) andhas_transforms, once per run.merge_raw(&TagInfo)stays as a thin public wrapper that buildsZipperTagsand delegates to a new internalmerge_raw_with; the hot per-template path inprocess_rawbuilds the lookups once and callsmerge_raw_withdirectly. The public API and thefgumi-umiTagInfotype are unchanged.Measured impact (M3 Ultra, aarch64)
HashSet<[u8;2]>(which still hashes).remove_tag+append, neg strand): −23% (three runs, 20–26%). The loop is otherwise dominated byremove_tag+append over the consensus aux, so the probe is ~a quarter of it.Behavior
Tags are matched by their raw two bytes. For all spec-conforming SAM tags (ASCII
[A-Za-z][A-Za-z0-9]) this is identical to the oldfrom_utf8-based probe. The only divergence is for a non-UTF-8 tag byte pair (reachable only from a malformed BAM) combined with an empty-string filter: the old path aliased it to""and could match an empty filter, whereas here the raw bytes are matched directly. This is the intended behavior; it is documented onTagBitsetand pinned by a test.Validation
--pedantic -D warnings+ the bare-tag-literal check all green. Added unit tests forTagBitsetmembership (case-sensitivity, full 0..=255 byte range, two-byte filtering, empty-filter semantics) andZipperTags::from_tag_info.SRR6109273.aligned.bam+.extract.fgbio.bam,-r hs38DH,-t 1) produce byte-identical records — the decompressed streams differ in exactly 46 of 393,397,066 bytes, reproducibly, and those 46 bytes are entirely the@PG VN:build commit hash. (Zipper output is deterministic: two runs of the same binary to the same output path are byte-identical — an earlier "1-byte per-run wobble" note was a measurement artifact, the differing byte being the-ofilename recorded verbatim in the@PG CL:field.)Risk: command output changes — none;
unsafechanges — none, and theCLAUDE.mdallowlist is unchanged; memory bounds, queue capacity, and thread/backpressure policy changes — none. The raw-byteTagBitsetpreserves existing two-byte tag filtering and transformation behavior. Tests cover membership, byte ranges, invalid-length filters, and lookup construction.