Repository navigation
perf(zipper): build the merge tag bitsets once per step, not per template - #917
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: Essentials Run ID: 📒 Files selected for processing (2)
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. WalkthroughRisk: template merges rebuilt tag lookups repeatedly. Fix: cache ChangesZipper tag lookup reuse
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Zipper tag lookups are now reused across template merges to reduce processing overhead while retaining the existing merge behavior. Both zipper and alignment paths are covered for multi-template tag transformations, 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 #917 +/- ##
==========================================
- Coverage 93.55% 93.55% -0.01%
==========================================
Files 301 301
Lines 150626 150735 +109
==========================================
+ Hits 140923 141020 +97
- Misses 9703 9715 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
What
Build the zipper merge tag bitsets (
ZipperTags— threeTagBitsetallocations derived fromcfg.tag_info) once per step instead of once per template.cfg.tag_infois fixed for a step's whole lifetime, so rebuildingZipperTagsinside the per-template merge loop allocated and populated three bitsets for every template with no change in result. Both zipper hot paths are fixed:AlignAndMergeStep(zipper spawns the aligner) holdsArc<ZipperTags>built once innew().ZipperMergeStep::emit_merged(standalonefgumi zipper -i mapped -u unmapped) caches it and drivesmerge_one_template_with.The dead
merge_one_template(&TagInfo)per-call wrapper is removed andmerge_raw_withreverted to private.Measured
Mac Studio (M3 Ultra, arm64), 8 threads, CPU-seconds via
/usr/bin/time -l, 3 reps, byte-identicalsamtools view | md5verified:fgumi zipperover 12.0M reads (6M templates; uBAM fromfgumi extract, mapped stream frombwa-mem3),--tags-to-reverse cd,ce,ad,ae,bd,be,aq,bq --tags-to-revcomp ac,bc: baseline 47.65 → branch 47.28 CPU-s, −0.8%, branch consistently ≤ baseline across reps. Output byte-identical.Small — the per-template allocation is a minor slice of zipper's total (BGZF codec + tag merge dominate) — but real, byte-identical, and removes an obvious per-template allocation from both hot paths.
Tests
Regression guard that
ZipperMergeStep::newbuildsZipperTagsonce andemit_mergedreuses the cached value across multiple(unmapped, mapped)pairs, driving the exact cached pathemit_mergeduses. Full gate green.Risk: grouping, consensus, sort order, corrected UMI, and metrics output changes are none; zipper output is byte-identical, covered by regression tests and the full test gate.
unsafechanges are none, and CLAUDE.md needs no allowlist update. Memory bounds, queue capacity, and backpressure policy changes are none.Cache
ZipperTagsonce per merge step and reuse them across template pairs throughArcandmerge_one_template_with. This reduces standalone zipper CPU time by 0.8%.