Skip to content

perf(sort): reuse buffers in merge phase via producer-consumer buffer pool - #219

Merged
nh13 merged 1 commit into
mainfrom
nh/perf-merge-buffer-reuse
Apr 2, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/perf-merge-buffer-reuse

Conversation

@nh13

@nh13 nh13 commented Apr 2, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Add a buffer return channel to GenericKeyedChunkReader so the consumer
    can return spent Vec<u8> buffers to the producer thread for reuse. The
    producer calls try_recv (non-blocking) to grab a recycled buffer before
    each record read, falling back to a fresh allocation only when the pool is
    empty. This eliminates per-record heap allocation in the merge phase once the
    pipeline warms up.
  • Changed next_record on all ChunkSource wrappers to accept a &mut Vec<u8>
    buffer parameter. Merge loops now write directly from each heap entry's record
    buffer and refill it in-place.
  • Removed the intermediate output_buffer: Vec<Vec<u8>> batching layer that
    previously collected 2048 owned Vecs before flushing.
  • Updated all four merge sites: consolidation, template-coordinate, generic, and
    indexed merge.

Test plan

  • cargo ci-fmt passes
  • cargo ci-lint passes
  • cargo nextest run --no-fail-fast — all 1735 tests pass
  • Benchmark on a representative BAM to confirm merge-phase allocation reduction

@nh13
nh13 temporarily deployed to github-actions April 2, 2026 21:33 — with GitHub Actions Inactive
@codecov

codecov Bot commented Apr 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.13043% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.06%. Comparing base (f13e78b) to head (9ac7632).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/sort/raw.rs 89.13% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
- Coverage   88.07%   88.06%   -0.02%     
==========================================
  Files         113      113              
  Lines       52863    52804      -59     
==========================================
- Hits        46561    46503      -58     
+ Misses       6302     6301       -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.

@nh13
nh13 marked this pull request as ready for review April 2, 2026 21:41
@coderabbitai

coderabbitai Bot commented Apr 2, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The GenericKeyedChunkReader::next_record method was refactored to accept a mutable external byte buffer and return only the sort key. Record bytes are now swapped into the provided buffer instead of allocating new vectors. Merge operations in maybe_consolidate_temp_files, merge_chunks_keyed, merge_chunks_generic, and merge_chunks_with_index were updated to reuse buffers in-place. The keyed merge flow was reworked to eliminate intermediate output buffer batching, writing records directly from heap entry buffers. Related internal next_record helpers across source enums were updated to match the buffer-reuse pattern.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed Title clearly describes the main change: buffer reuse optimization in the merge phase using a producer-consumer buffer pool pattern.
Description check ✅ Passed Description is directly related to the changeset, detailing buffer reuse mechanisms, API changes, and updated merge sites.

✏️ 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/perf-merge-buffer-reuse

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.

@nh13 nh13 added the fgum sort label Apr 2, 2026
@nh13

nh13 commented Apr 2, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 2, 2026

Copy link
Copy Markdown
✅ 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 coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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/sort/raw.rs`:
- Around line 423-431: The implementation of next_record(&mut self, buf: &mut
Vec<u8>) only swaps vectors but doesn't enable real buffer reuse because
producers allocate fresh Vec<u8> per record (see read_records) and consumed
caller buffers get dropped/parked (forward-only memory source using idx); to
fix, change the producer APIs (e.g., read_records and any producer filling the
channel used by receiver) to write into caller-owned buffers or to return empty
buffers back into a reusable pool instead of allocating new Vecs, and update the
receiver/next_record code to accept and forward these pooled/filled buffers
(keep symbols: next_record, receiver.recv, read_records, idx) so that the
swapped-out buffer is reused by the producer rather than dropped.
🪄 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: 59d722a7-eba7-4fcb-92dc-bca87002d67e

📥 Commits

Reviewing files that changed from the base of the PR and between f13e78b and f20deba.

📒 Files selected for processing (1)
  • src/lib/sort/raw.rs

Comment thread src/lib/sort/raw.rs
Change GenericKeyedChunkReader::next_record and all ChunkSource
wrappers to accept a &mut Vec<u8> buffer parameter instead of
returning an owned Vec<u8>. This lets merge loops write directly
from each heap entry's buffer and refill it in-place, avoiding
per-record Vec allocations and the intermediate output_buffer
that previously collected owned Vecs before flushing.
@nh13
nh13 force-pushed the nh/perf-merge-buffer-reuse branch from f20deba to 9ac7632 Compare April 2, 2026 22:14
@nh13
nh13 temporarily deployed to github-actions April 2, 2026 22:14 — with GitHub Actions Inactive
@nh13 nh13 changed the title Reuse buffers in merge phase to reduce allocations perf(sort): reuse buffers in merge phase via producer-consumer buffer pool Apr 2, 2026
@nh13
nh13 merged commit 2f511cf into main Apr 2, 2026
6 of 7 checks passed
@nh13
nh13 deleted the nh/perf-merge-buffer-reuse branch April 2, 2026 22:32
@nh13 nh13 mentioned this pull request Apr 2, 2026

This branch was previously deployed

1 inactive deployment
github-actions — 9ac76329 Deployed Apr 2, 2026 by nh13 via coverage #832
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