Skip to content

bench: reuse multiline terminator Finder - #115

Closed
mjc wants to merge 1 commit into
mainfrom
bench/multiline-terminator-finder
Closed

mjc wants to merge 1 commit into
mainfrom
bench/multiline-terminator-finder

Conversation

@mjc

@mjc mjc commented Sep 13, 2026

Copy link
Copy Markdown
Owner

Summary

Reuses one process-global memchr::memmem::Finder for the fixed multiline terminator. The framer still owns all boundary handling; candidate collection, suffix checks, and split-read behavior are unchanged.

E2E benchmark evidence

Measurements used an AMD Ryzen 9 5950X (32 logical CPUs), main commit 145b36c6b86621dccfe965926b0ef501ffd33b06, and Finder source revision e176fef31432ed36dc101471dfa1c94cc1650908.

Each run is the stock cache-miss E2E script after a whole-target clean, with RUSTFLAGS=-C target-cpu=native. It builds the measured proxy and pinned nntpbench itself, then exercises all 112 default cells: 10 GiB per cell, 728,320-byte article responses, article-only mix, pipeline depth 32, and the 1/2/4/8 thread × 1/2/4/8/16/32/64 backend-connection × 1/4/8/16 client matrix.

Fresh main averaged 3,546.6 MiB/s. Two clean Finder repetitions averaged 3,564.3 MiB/s (+0.5%) and 3,560.8 MiB/s (+0.4%). Their paired medians were -0.4% and +0.4%; geometric means were +0.6% and -0.5%; win/loss counts were 50/62 and 63/49. Proxy CPU changed -0.8% and +0.2%.

The repeats are effectively flat with broad per-cell variance, so they do not demonstrate a reliable end-to-end speedup. The CSVs are retained locally rather than committed.

The commit hook ran cargo fmt and cargo clippy successfully; the branch-added benchmark note documents the workload and both repetitions.

Summary by CodeRabbit

  • Performance

    • Improved processing efficiency for multiline message terminators by reusing a cached search pattern.
  • Documentation

    • Added benchmark results comparing the updated implementation with the previous version on a cache-miss end-to-end run.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 1e7d94c5-c4d1-41e7-984f-e092e0a57675

📥 Commits

Reviewing files that changed from the base of the PR and between e176fef and 33a1e60.

📒 Files selected for processing (1)
  • docs/development.md

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The module now lazily caches a memchr::memmem::Finder for the NNTP terminator. terminator_ends reuses the finder. Development documentation now records benchmark measurements and their limitations.

Changes

Terminator Search Optimization

Layer / File(s) Summary
Cache and reuse the terminator finder
src/session/multiline_framing.rs
The module adds a lazily initialized terminator finder and uses it in terminator_ends instead of creating an inline search iterator.
Record benchmark results
docs/development.md
The Benchmarks section now includes measured control and Finder candidate results, comparison statistics, CPU changes, and notes that the results do not demonstrate a reliable end-to-end speedup.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to 33a1e

The cached Finder is compatible with the repository toolchain and preserves the tested framing semantics; the documented benchmark results are appropriately qualified.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. (1 skipped: 1 …
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: reusing the multiline terminator Finder. It matches the implementation and benchmark objective.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bench/multiline-terminator-finder

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

A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.

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

@mjc
mjc force-pushed the bench/multiline-terminator-finder branch from 7901e19 to 3743ac1 Compare September 13, 2026 21:53
@mjc
mjc force-pushed the bench/multiline-terminator-finder branch from 3743ac1 to 33a1e60 Compare September 13, 2026 22:20
@mjc mjc closed this Sep 13, 2026
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