perf(zipper): speed up --restore-unconverted-bases hot path by nh13 · Pull Request #271 · fulcrumgenomics/fgumi

nh13 · 2026-04-14T23:28:22Z

Summary

ReferenceReader::fetch_slice returns &[u8] instead of Vec<u8>; eliminates ~2 GB of Vec allocation churn over a 13M-template run. fetch becomes a thin wrapper for callers that need owned bytes.
Borrow the contig name from the BAM header instead of String-allocating per record.
Early-exit restore_unconverted_bases_in_record via memchr::memchr2 when the aligned reference span has no candidate base — skips both the SEQ allocation and the CIGAR walk.
Precompute the lowercase variants of the target/converted bases once per record so the inner loop avoids to_ascii_uppercase() per byte.
Defer the SEQ Vec<u8> allocation until the first base actually needs to change. Reads aligned to ref-C positions but already carrying C in the read (common for highly-methylated CpGs in EM-Seq) now return without allocating SEQ at all.

Why

fgumi zipper --restore-unconverted-bases (added in #170) drops the streaming raw-byte merge path used by TAPs/non-methyl in favour of a RecordBuf round-trip so it can fetch reference bases per record. Profiling a real 26.8M-record EM-Seq run showed ~46K records/s — about 10× slower than bwa mem | bwameth.py can produce. With default bwa -K 150000000, bwameth.py accumulates a chunk's worth of Bam objects in Python while waiting on zipper, eventually getting OOM-killed mid-pipe.

The five changes here all sit on the per-record hot path and stack cleanly. None of them change observable output for any record that has bases to restore — the existing 9 restore tests still pass byte-for-byte. The new no_target_in_span test locks in the early-exit, and test_restore_unconverted_bases_preserves_already_unconverted already exercises the deferred-SEQ-allocation path (it asserts the no-change case, which now also incurs no allocation).

End-to-end speedup will be measured in the next fgumi-benchmarks methylation run; the alloc/copy reductions are mechanical and the early-exit + deferred-allocation are unconditional skips for any read that doesn't need work.

Test plan

New unit test test_fetch_slice_returns_borrowed_bytes covers &[u8] return, byte-equality with fetch, and bounds errors.
New unit test test_restore_unconverted_bases_no_target_in_span covers the memchr2 early-exit fast path.
Existing 9 test_restore_unconverted_bases_* tests pass byte-for-byte (no behavioral regressions).
Full workspace cargo ci-test: 2495 passed, 19 skipped.
cargo ci-fmt and cargo ci-lint (clippy pedantic + -D warnings) clean.

coderabbitai · 2026-04-14T23:28:28Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0b566f79-dd19-4807-a13d-fe14185166e9

📥 Commits

Reviewing files that changed from the base of the PR and between 679bd0a and f2781e7.

📒 Files selected for processing (3)

crates/fgumi-sam/src/tag.rs
src/lib/commands/zipper.rs
src/lib/reference.rs

🚧 Files skipped from review as they are similar to previous changes (1)

src/lib/reference.rs

📝 Walkthrough

Walkthrough

The PR adds ReferenceReader::fetch_slice returning a borrowed &[u8], updates fetch to call it, and tightens region validation to treat empty ranges as invalid. restore_unconverted_bases_in_record now borrows contig names, uses fetch_slice, precomputes case candidates, fast-exits when no targets, lazily allocates SEQ only on mutation, and only removes NM/MD tags when SEQ changed. Adds SamTag::MD and unit tests covering the new behaviors.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title accurately summarizes the main performance optimization focus of the changeset—speeding up the hot path in the --restore-unconverted-bases feature.
Description check	✅ Passed	Description thoroughly explains all five changes, their performance impact, and rationale—directly aligned with the changeset's objectives and modifications.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch nh/zipper-restore-perf

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

codecov · 2026-04-14T23:30:38Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.88%. Comparing base (25ede93) to head (f2781e7).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #271      +/-   ##
==========================================
+ Coverage   89.84%   89.88%   +0.03%     
==========================================
  Files         120      120              
  Lines       61359    61585     +226     
==========================================
+ Hits        55127    55354     +227     
+ Misses       6232     6231       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

`fgumi zipper --restore-unconverted-bases` (added in #170) was the only zipper code path that dropped the streaming raw-byte merge in favour of a `RecordBuf` round-trip — full BAM parse, per-field allocation, per-record mutation, then re-encode every byte. That round-trip is the dominant cost on the methylation path. This brings the methylation path onto the same raw-byte fast path TAPs already uses. The dispatch in `process_raw` is now uniform — convert_template_to_raw(...); merge_raw(...); if let Some(ref_reader) = reference { restore_unconverted_bases_in_raw_template(..., ref_reader, ...); } — with the methylation case adding one extra step that mutates SEQ in place via `bam_fields::set_base`. SEQ stays the same length (4-bit nibble rewrite), so no record resizing is needed; only `NM` / `MD` are removed when SEQ is actually modified. The new `restore_unconverted_bases_in_raw_record` reuses the `fgumi-raw-bam` accessors catalogued in #272 (`flags`, `pos`, `ref_id`, `get_cigar_ops`, `cigar_op_kind`, `find_string_tag_in_record` for `YD`, `seq_offset`, `get_base`, `set_base`, and `remove_tag`). It also keeps the perf wins from #271 — borrowed-slice `fetch_slice`, precomputed lowercase target, `memchr2`-based early-exit — so reads aligned to spans without candidate ref bases skip the CIGAR walk entirely. The `RecordBuf` reference implementation is kept under `cfg(test)` as an oracle: every existing `test_restore_unconverted_bases_*` test still passes against it, and six new `test_restore_unconverted_bases_in_raw_record_*` tests cover the same scenarios on the raw-byte path (top strand, bottom strand, reverse top strand, no-`YD` skip, unmapped skip, `NM` / `MD` stale-tag clearing). Stacks on #271 for `ReferenceReader::fetch_slice`. Rebase onto `main` once #271 lands. Full workspace `cargo ci-test`: passes. `cargo ci-fmt` and `cargo ci-lint` (clippy pedantic + `-D warnings`) clean.

nh13 · 2026-04-15T04:13:25Z

@coderabbitai review

coderabbitai · 2026-04-15T04:13:32Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

nh13 · 2026-04-15T05:12:36Z

@coderabbitai review

coderabbitai · 2026-04-15T05:12:42Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/lib/reference.rs (1)

291-313: ⚠️ Potential issue | 🟡 Minor

Invalid interval guard allows start = end + 1 to pass.

start_idx > end_idx misses the adjacent inverted case and returns an empty slice instead of erroring.

Suggested fix

-        if end_idx > sequence.len() || start_idx > end_idx {
+        if end_idx > sequence.len() || start_idx >= end_idx {
             return Err(FgumiError::InvalidParameter {
                 parameter: "region".to_string(),
                 reason: format!(

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/reference.rs` around lines 291 - 313, In fetch_slice, the interval
check uses "if end_idx > sequence.len() || start_idx > end_idx" which lets the
adjacent inverted case (start = end + 1) slip through; change the second check
to "start_idx >= end_idx" so any non-positive-length or inverted interval
(start_idx >= end_idx) returns the InvalidParameter error; update the
conditional referencing start_idx and end_idx in the fetch_slice method
accordingly.

🧹 Nitpick comments (1)

src/lib/reference.rs (1)

414-439: Add a regression assertion for inverted intervals (start > end).

This test covers missing-contig and out-of-bounds, but not the start > end case that exercises interval validation directly.

Suggested test addition

         assert!(
             reader
                 .fetch_slice("chr1", Position::try_from(1)?, Position::try_from(10_000)?)
                 .is_err()
         );
+        assert!(
+            reader
+                .fetch_slice("chr1", Position::try_from(5)?, Position::try_from(4)?)
+                .is_err()
+        );
         assert!(
             reader.fetch_slice("nope", Position::try_from(1)?, Position::try_from(2)?).is_err()
         );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/reference.rs` around lines 414 - 439, The test
test_fetch_slice_returns_borrowed_bytes is missing a regression assertion for
inverted intervals; add an assertion that calling reader.fetch_slice with start
> end (e.g., Position::try_from(4) as start and Position::try_from(1) as end)
returns an Err, mirroring how fetch should behave on invalid intervals—place
this assertion alongside the existing bounds and missing-contig checks to ensure
fetch_slice validates interval ordering the same way as fetch.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/lib/commands/zipper.rs`:
- Around line 4049-4084: The test
test_restore_unconverted_bases_no_target_in_span currently doesn't verify NM/MD
preservation because the Record lacks those tags; update the RecordBuilder setup
to include explicit NM and MD tags (e.g., set NM and MD via the builder or add
them to the Record data) before calling restore_unconverted_bases_in_record,
then after the call assert that the NM and MD tags (Tag::new(b'N', b'M') and
Tag::new(b'M', b'D')) still exist and equal the original values to prove they
were preserved on a no-op.

---

Outside diff comments:
In `@src/lib/reference.rs`:
- Around line 291-313: In fetch_slice, the interval check uses "if end_idx >
sequence.len() || start_idx > end_idx" which lets the adjacent inverted case
(start = end + 1) slip through; change the second check to "start_idx >=
end_idx" so any non-positive-length or inverted interval (start_idx >= end_idx)
returns the InvalidParameter error; update the conditional referencing start_idx
and end_idx in the fetch_slice method accordingly.

---

Nitpick comments:
In `@src/lib/reference.rs`:
- Around line 414-439: The test test_fetch_slice_returns_borrowed_bytes is
missing a regression assertion for inverted intervals; add an assertion that
calling reader.fetch_slice with start > end (e.g., Position::try_from(4) as
start and Position::try_from(1) as end) returns an Err, mirroring how fetch
should behave on invalid intervals—place this assertion alongside the existing
bounds and missing-contig checks to ensure fetch_slice validates interval
ordering the same way as fetch.

🪄 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: c76703a5-d05c-4578-a0fc-603cf0011a4e

📥 Commits

Reviewing files that changed from the base of the PR and between 25ede93 and 679bd0a.

📒 Files selected for processing (2)

src/lib/commands/zipper.rs
src/lib/reference.rs

`restore_unconverted_bases_in_record` is called once per mapped record and dominates throughput of `fgumi zipper --restore-unconverted-bases`. Profiling a 26.8M-record EM-Seq run showed ~46K records/s, about 10x slower than upstream bwa+bwameth can produce, causing bwameth.py to back up and OOM-kill its `c2t` worker under default `bwa -K` chunks. Five changes that touch the same hot loop: 1. ReferenceReader::fetch_slice — new borrowed-slice API. `fetch` already kept the entire reference resident in memory but returned a fresh `Vec<u8>` per call. For a 13M-template run that's ~2 GB of allocator churn. `fetch_slice` returns `&[u8]` into the in-memory buffer; `fetch` is now a thin wrapper. 2. Borrow the contig name from the BAM header instead of allocating a `String` per record. One fewer alloc per record on the hot path. 3. Early-exit when the aligned reference span contains no candidate base (C for top-strand, G for bottom-strand). Uses `memchr::memchr2` for SIMD-accelerated detection of upper- and lower-case targets in one pass; skips the SEQ allocation and CIGAR walk entirely. 4. Hoist case-folding out of the inner loop. Previously the per-base compare did `to_ascii_uppercase()` on both ref and read bytes; precompute the lowercase variants once per record and just check equality against both. 5. Defer the SEQ `Vec<u8>` allocation until the first base actually needs to change. Reads that align to ref-C positions but already carry C in the read (highly methylated CpGs in EM-Seq, common case) now leave the function without allocating a SEQ buffer at all. Adds `test_restore_unconverted_bases_no_target_in_span` to lock in the early-exit, plus `test_fetch_slice_returns_borrowed_bytes` for the new API. The existing `test_restore_unconverted_bases_preserves_already_unconverted` already exercises the deferred-allocation path. All other tests pass unchanged (90 directly-affected reference + restore tests; full workspace 2495 pass, 19 skipped).

nh13 · 2026-04-15T06:13:07Z

@coderabbitai review

coderabbitai · 2026-04-15T06:13:15Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

`fgumi zipper --restore-unconverted-bases` (added in #170) was the only zipper code path that dropped the streaming raw-byte merge in favour of a `RecordBuf` round-trip — full BAM parse, per-field allocation, per-record mutation, then re-encode every byte. That round-trip is the dominant cost on the methylation path. This brings the methylation path onto the same raw-byte fast path TAPs already uses. The dispatch in `process_raw` is now uniform — convert_template_to_raw(...); merge_raw(...); if let Some(ref_reader) = reference { restore_unconverted_bases_in_raw_template(..., ref_reader, ...); } — with the methylation case adding one extra step that mutates SEQ in place via `bam_fields::set_base`. SEQ stays the same length (4-bit nibble rewrite), so no record resizing is needed; only `NM` / `MD` are removed when SEQ is actually modified. The new `restore_unconverted_bases_in_raw_record` reuses the `fgumi-raw-bam` accessors catalogued in #272 (`flags`, `pos`, `ref_id`, `get_cigar_ops`, `cigar_op_kind`, `find_string_tag_in_record` for `YD`, `seq_offset`, `get_base`, `set_base`, and `remove_tag`). It also keeps the perf wins from #271 — borrowed-slice `fetch_slice`, precomputed lowercase target, `memchr2`-based early-exit — so reads aligned to spans without candidate ref bases skip the CIGAR walk entirely. The `RecordBuf` reference implementation is kept under `cfg(test)` as an oracle: every existing `test_restore_unconverted_bases_*` test still passes against it, and six new `test_restore_unconverted_bases_in_raw_record_*` tests cover the same scenarios on the raw-byte path (top strand, bottom strand, reverse top strand, no-`YD` skip, unmapped skip, `NM` / `MD` stale-tag clearing). Stacks on #271 for `ReferenceReader::fetch_slice`. Rebase onto `main` once #271 lands. Full workspace `cargo ci-test`: passes. `cargo ci-fmt` and `cargo ci-lint` (clippy pedantic + `-D warnings`) clean.

nh13 added the fgumi zipper label Apr 14, 2026

nh13 temporarily deployed to github-actions April 14, 2026 23:28 — with GitHub Actions Inactive

nh13 mentioned this pull request Apr 14, 2026

Complete the raw-byte BAM API in fgumi-raw-bam #272

Closed

9 tasks

nh13 mentioned this pull request Apr 14, 2026

perf(zipper): restore unconverted bases on raw BAM bytes #274

Merged

5 tasks done

coderabbitai Bot requested changes Apr 15, 2026

View reviewed changes

Comment thread src/lib/commands/zipper.rs

coderabbitai Bot approved these changes Apr 15, 2026

View reviewed changes

nh13 force-pushed the nh/zipper-restore-perf branch from 679bd0a to f2781e7 Compare April 15, 2026 05:47

nh13 temporarily deployed to github-actions April 15, 2026 05:47 — with GitHub Actions Inactive

nh13 merged commit 3854d5c into main Apr 15, 2026
9 checks passed

nh13 deleted the nh/zipper-restore-perf branch April 15, 2026 07:37

nh13 mentioned this pull request Apr 15, 2026

chore: release v0.2.0 #259

Merged

This was referenced Apr 17, 2026

refactor(lib): migrate commands, storage, and pipeline to raw-byte records #292

Merged

chore: release v0.3.0 #309

Merged

Navigation Menu

Repository navigation

perf(zipper): speed up --restore-unconverted-bases hot path - #271

perf(zipper): speed up --restore-unconverted-bases hot path#271
nh13 merged 1 commit into
mainfrom
nh/zipper-restore-perf

nh13 commented Apr 14, 2026

Uh oh!

coderabbitai Bot commented Apr 14, 2026 •

edited

Loading

Walkthrough

Uh oh!

codecov Bot commented Apr 14, 2026 •

edited

Loading

Uh oh!

nh13 commented Apr 15, 2026

Uh oh!

coderabbitai Bot commented Apr 15, 2026

Uh oh!

nh13 commented Apr 15, 2026

Uh oh!

coderabbitai Bot commented Apr 15, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

nh13 commented Apr 15, 2026

Uh oh!

coderabbitai Bot commented Apr 15, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

nh13 commented Apr 14, 2026

Summary

Why

Test plan

Uh oh!

coderabbitai Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Uh oh!

codecov Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nh13 commented Apr 15, 2026

Uh oh!

coderabbitai Bot commented Apr 15, 2026

Uh oh!

nh13 commented Apr 15, 2026

Uh oh!

coderabbitai Bot commented Apr 15, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nh13 commented Apr 15, 2026

Uh oh!

coderabbitai Bot commented Apr 15, 2026

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Apr 14, 2026 •

edited

Loading

codecov Bot commented Apr 14, 2026 •

edited

Loading