Skip to content

perf(pipeline): dedupe the TemplatesToMiGroups bridge and stop cloning records - #431

Merged
nh13 merged 2 commits into
feat-runallfrom
feat-runall-31-mi-bridge-clone
Jun 18, 2026
Merged

nh13 merged 2 commits into
feat-runallfrom
feat-runall-31-mi-bridge-clone

Conversation

@nh13

@nh13 nh13 commented Jun 18, 2026

Copy link
Copy Markdown
Member

Wave 2 perf cleanup (#8 of the feat-runall review series) — the highest-value item: it hits the primary runall workload (fused sort→group→consensus / group→consensus). Two commits, read in order.

1. refactor(pipeline): extract the bridge into one factory

add_simplex, add_duplex, and add_codec each inlined a ~70-line copy of the TemplatesToMiGroups bridge (the fused group→consensus path that splices each template's MI into the MI tag and runs records into per-MI groups). The three copies were identical except for one flag — duplex_strip_strand (true for duplex, false for simplex/codec). Hoisted into a single templates_to_mi_step(limit_bytes, duplex_strip_strand) factory called from all three sites. Pure dedup, no behavior change.

2. perf(pipeline): move tagged bytes into records instead of cloning

The bridge tags each record's MI in a reused scratch buffer, then did RawRecord::from(state.scratch.clone()) — a full per-record heap copy on top of the extend_from_slice, once per read on the main runall workload. Now moves the buffer into the record with std::mem::replace, reinstalling a fresh Vec pre-sized to the previous capacity so the next record reuses it without a realloc (the per-record clear() keeps it warm). Same allocation count as the clone, minus the record-body memcpy. Output unchanged.

Verification

  • cargo ci-fmt / cargo ci-lint (clippy pedantic) clean.
  • cargo nextest -E 'test(runall) or test(group_determinism)' — 149 tests pass, covering all three fused group→{simplex,duplex,codec} shapes and MI determinism.
  • Net −132 lines.
  • CodeRabbit CLI (--agent) and a CodeRabbit-style self-review: 0 findings.

nh13 added 2 commits June 17, 2026 17:14
…actory

`add_simplex`, `add_duplex`, and `add_codec` each inlined a ~70-line copy of
the `TemplatesToMiGroups` bridge step (the fused group→consensus path that
splices each template's MI into the `MI` tag and runs records into per-MI
groups). The three copies were identical except for one flag —
`duplex_strip_strand` (true for duplex, false for simplex/codec).

Hoist the closure into a single `templates_to_mi_step(limit_bytes,
duplex_strip_strand)` factory and call it from all three sites. No behavior
change; this is a pure deduplication so the upcoming clone-removal lands once.
…loning

The `TemplatesToMiGroups` bridge tags each record's MI in a reused `scratch`
buffer, then did `RawRecord::from(state.scratch.clone())` — a full per-record
heap copy on top of the `extend_from_slice`. This runs once per read on the
primary runall workload (fused sort→group→consensus / group→consensus).

Move the buffer into the record with `std::mem::replace`, reinstalling a fresh
`Vec` pre-sized to the previous capacity so the next record reuses it without a
realloc (the per-record `clear()` keeps it warm). Same allocation count as the
clone, minus the record-body memcpy. Output is unchanged.
@nh13
nh13 temporarily deployed to github-actions June 18, 2026 00:15 — with GitHub Actions Inactive
@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (feat-runall@f9e7817). Learn more about missing BASE report.

Additional details and impacted files
@@              Coverage Diff               @@
##             feat-runall     #431   +/-   ##
==============================================
  Coverage               ?   93.40%           
==============================================
  Files                  ?      139           
  Lines                  ?    54817           
  Branches               ?        0           
==============================================
  Hits                   ?    51200           
  Misses                 ?     3617           
  Partials               ?        0           

☔ View full report in Codecov by Harness.
📢 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.

@nh13

nh13 commented Jun 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 18, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

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

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Review Change Stack

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: 0b5df7d1-bfaf-482a-a7b2-39caf7b9d2ad

📥 Commits

Reviewing files that changed from the base of the PR and between f9e7817 and 3065da4.

📒 Files selected for processing (1)
  • src/lib/pipeline/chains/builder.rs

📝 Walkthrough

Walkthrough

In builder.rs, a new module-level helper templates_to_mi_step(limit_bytes, duplex_strip_strand) is introduced. It encapsulates the process_with_worker_state step that converts BatchedProcessedPositionGroups to BatchedMiGroups: iterating templates, computing MI keys (stripping duplex strand when duplex_strip_strand = true, otherwise deriving the base MI string), updating raw MI tags via fgumi_raw_bam::update_string_tag, and emitting grouped MiGroups. The three inline copies of this logic in add_simplex, add_duplex, and add_codec are replaced with calls to the helper, reducing net line count by ~132 lines.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: deduplicating the TemplatesToMiGroups bridge and eliminating record cloning in the pipeline.
Description check ✅ Passed The description is directly related to the changeset, detailing the refactor and perf improvements with clear context, verification steps, and impact metrics.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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 feat-runall-31-mi-bridge-clone

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

@nh13
nh13 merged commit 18a2086 into feat-runall Jun 18, 2026
10 checks passed
@nh13
nh13 deleted the feat-runall-31-mi-bridge-clone branch June 18, 2026 04:53
nh13 added a commit that referenced this pull request Jun 18, 2026
…g records (#431)

* refactor(pipeline): extract the TemplatesToMiGroups bridge into one factory

`add_simplex`, `add_duplex`, and `add_codec` each inlined a ~70-line copy of
the `TemplatesToMiGroups` bridge step (the fused group→consensus path that
splices each template's MI into the `MI` tag and runs records into per-MI
groups). The three copies were identical except for one flag —
`duplex_strip_strand` (true for duplex, false for simplex/codec).

Hoist the closure into a single `templates_to_mi_step(limit_bytes,
duplex_strip_strand)` factory and call it from all three sites. No behavior
change; this is a pure deduplication so the upcoming clone-removal lands once.

* perf(pipeline): move tagged bytes into MI-bridge records instead of cloning

The `TemplatesToMiGroups` bridge tags each record's MI in a reused `scratch`
buffer, then did `RawRecord::from(state.scratch.clone())` — a full per-record
heap copy on top of the `extend_from_slice`. This runs once per read on the
primary runall workload (fused sort→group→consensus / group→consensus).

Move the buffer into the record with `std::mem::replace`, reinstalling a fresh
`Vec` pre-sized to the previous capacity so the next record reuses it without a
realloc (the per-record `clear()` keeps it warm). Same allocation count as the
clone, minus the record-body memcpy. Output is unchanged.

This branch was previously deployed

1 inactive deployment
github-actions — 3065da47 Deployed Jun 18, 2026 by nh13 via coverage #1682
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant