Repository navigation
perf(filter): single-pass scalar consensus-tag extraction (-9% CPU) - #969
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 (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. WalkthroughThe change adds single-pass consensus tag parsing and reuses parsed tags during simplex and duplex filtering. Existing threshold, error, masking, classification, and no-call outcomes remain covered by parity and boundary tests. ChangesConsensus tag filtering
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No identified behavior change currently presents a merge-blocking risk. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #969 +/- ##
==========================================
- Coverage 96.09% 96.08% -0.02%
==========================================
Files 292 292
Lines 145174 145691 +517
==========================================
+ Hits 139510 139986 +476
- Misses 5664 5705 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
34e2548 to
b46bc79
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
The read-level consensus filter walked a record's aux block ~9 times per record: is_duplex_consensus reads aD+bD, filter_read reads cD+cE, and filter_duplex_read re-reads cD+cE then aD/aM/bD/bM/aE/bE (aD/bD scanned up to 3x). Each find_*_tag restarts at offset 0, so cost was O(tags * aux_len)/record — the find_tag_position hot spot in the duplex-pipeline profile. Add ConsensusScalarTags::from_aux(): one O(aux_len) walk decoding each target scalar tag's value at its first occurrence, mirroring the existing extract_aux_string_tags single-pass pattern. New allocation-free filter_read_tags / filter_duplex_read_tags operate on it; the duplex tier comparison is factored into a shared duplex_tier_result() so the per-tag and single-pass paths cannot drift. The filter command hot path builds the struct once per record and routes through the _tags variants. Match the per-tag path exactly, including on malformed aux: a per-tag `seen` bitset locks each slot to its first occurrence regardless of decode outcome (mirroring find_tag_position) and records aD/bD presence independently of a successful integer decode, so is_duplex() matches is_duplex_consensus()'s type-agnostic presence check rather than diverging on a present-but-mistyped or duplicated tag. consensus_scalar_tags_match_find (well-formed combos) and consensus_scalar_tags_match_find_malformed (mistyped/duplicate aux) pin the equivalence; filter unit + consensus tests pass, clippy pedantic clean.
b46bc79 to
1bcf5de
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
Summary
The read-level consensus filter walked a record's aux block ~9 times per record, each
find_*_tagrestarting at offset 0:is_duplex_consensusreadsaD+bDfilter_readreadscD+cEfilter_duplex_readre-readscD+cE, thenaD/aM/bD/bM/aE/bEaD/bDwere scanned up to 3× per record, so the cost wasO(tags · aux_len)per record. Profiling the duplex pipeline showedfind_tag_positionas a hot spot (8.35% self-time in standalonefilter).This introduces
ConsensusScalarTags::from_aux()— a singleO(aux_len)walk that collects all eight scalar consensus tags in one pass, mirroring the existingextract_aux_string_tagspattern (and in the same spirit as #964's single-pass zipper walks). New allocation-freefilter_read_tags/filter_duplex_read_tagsoperate on the struct; the AB/BA tier comparison is factored into a sharedduplex_tier_result()so the per-tag and single-pass paths cannot drift. Thefiltercommand hot path builds the struct once per record and routes classification + both filter decisions through it.Performance (measured, c8g.8xlarge / 32 vCPU Graviton4, t16)
Standalone
fgumi filterover a 9,998,652-read duplex consensus BAM, 3 reps:perfself-time forfind_tag_position: 8.35% → 3.36% (~60% of the tag-scan cost removed; the residual is the per-base array tags inmask_duplex_bases, intentionally untouched).Correctness
Byte-identical to the per-tag path — output unchanged (kept 9,998,652 every run):
consensus_scalar_tags_match_findasserts the single-pass struct +_tagsfilters equal the per-tagfind_*results across 7 tag combinations (simplex, full duplex,aM/bMfallback, missing pairs, absent-cD, interleaved/out-of-order tags).check_filters_rawtests pass; clippy clean.Notes
The same single-pass technique does not apply to the duplex/simplex/codec callers: those hot paths read exactly one tag per record (MI in strand partitioning, RX per source read), so there is no multi-tag re-scan to collapse. This PR is scoped to
filter, where the redundancy actually was.Risk: output changes — none;
unsafechanges — none, with no CLAUDE.md allowlist update required; memory, queue, and thread/backpressure policy changes — none.Fix: parse the eight scalar consensus tags once per record and reuse them for classification and filtering.
ConsensusScalarTags::from_aux()with matching first-occurrence, presence, and malformed-aux semantics.filter_read_tags()andfilter_duplex_read_tags().filtercommand.