perf(socket): release the batch buffer a burst grew once the burst ends - #1246
Conversation
One large stanza grows the sender's batch buffer to its own size, and split() hands the written bytes over while the allocation stays behind, so the buffer keeps that high-water mark for the rest of the connection. Measured per socket in release, 16 sockets: 8.2 KiB after small traffic only, 60.2 KiB after a single 60 KiB frame. A session that sends one media-sized stanza pays 52 KiB for the rest of its life. The buffer is now replaced with a fresh idle-sized one after 32 consecutive small batches. That is off the batching path entirely: it runs after the write, where split() has already left out_buf empty, so no ceiling check, carry-over or waiter sees it. Sustained large traffic never reaches the threshold and never shrinks; the cost lands only on a session that goes quiet and then bursts again, which pays the deque growth once. Marginal after a 60 KiB frame plus quiet traffic: 60.2 -> 8.5 KiB, level with a socket that never sent a large frame at all.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe noise socket sender now adapts its batch-buffer capacity. It starts at 4 KiB, tracks oversized batches, reclaims the buffer after 32 small batches, and regrows it when required. An end-to-end test validates batching and payload integrity. ChangesAdaptive batching buffer management
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
| Filename | Overview |
|---|---|
| src/socket/noise_socket.rs | Adds post-burst batch-buffer release state and accompanying sender-path tests. |
Reviews (3): Last reviewed commit: "test(socket): tie the grown-capacity fix..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/socket/noise_socket.rs`:
- Around line 1206-1264: Extract the buffer reclamation policy around the
existing buffer transition into a testable helper, then add direct assertions
that a grown buffer is retained through small batch 31 and replaced by an
idle-capacity buffer after small batch 32. Keep the current end-to-end batching
and encryption test unchanged, including its wire-level assertions.
🪄 Autofix
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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ae7dbc09-18bc-46ba-873a-638a9b99bc47
📒 Files selected for processing (1)
src/socket/noise_socket.rs
📦 Binary size report
.text per crate
Baseline: |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 1 file
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…wire Two review findings, both valid. A frame that fails mid-encrypt is truncated back out of the buffer, so the allocation it grew survives with no wire bytes naming it and the countdown never started. The decision now reads the buffer's capacity as well as the batch length. Capacity only marks the buffer as grown, though: letting it also restart the countdown would mean a grown buffer perpetually resetting the very timer meant to release it. The wire-level test could not observe any of this — out_buf and its two counters live inside the sender task, so deleting the whole shrink left every assertion in the file green. The decision is now a free function with its own tests, verified by stubbing it out: two of the four fail when it does nothing.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The truncated-frame case hard-coded 64 KiB, so a change to OUT_BUF_IDLE_CAPACITY could take the fixture under the threshold and quietly stop testing anything. Derived from the constant like its siblings.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: The change introduces a memory-vs-CPU tradeoff: a 32-batch quiet threshold and 4 KiB idle capacity decide when to release a grown buffer, forcing reallocation on resumed bursts. That perf/capacity tuning choice needs human sign-off.
Re-trigger cubic
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
Target 4 of the per-session marginal batch, and the largest of the five: 52 KiB per session, paid by any session that sends one large stanza.
#1235left this one out of the report because the batch buffer is a local inside a spawned task and nothing can read it from outside. That is still true, but it turns out not to matter — the socket's own mock-transport scaffolding drives the sender end to end, so residency is measurable fromRssAnonwithout a live WhatsApp session. Median over 16 sockets, release:One media-sized stanza raises the socket's floor by 52 KiB and it never comes back down for the life of the connection.
Design
out_buf.split()hands the written bytes to the transport and leavesout_bufempty — but both halves share the allocation, soout_bufkeeps a handle to it, and once theBytesis dropped the nextreservereclaims the whole thing. That is what makes the batching zero-copy, and it is also what makes the high-water mark permanent: the buffer never gives back what one big frame made it grow to.The fix is to notice when the burst is over and start again from an idle-sized buffer. After 32 consecutive small batches,
out_bufis replaced with a freshBytesMut::with_capacity(4096)and the grown allocation is freed.Why this cannot regress batching. The reset runs after the write completes, at a point where
split()has already leftout_bufempty and the waiters have been answered. No ceiling check, no carry-over decision, and no waiter observes it — the coalescing loop is not touched at all. The29% fewer writes against 3.7% pong latencytradenode_io.rsdocuments is unaffected because nothing about when or how frames are batched changed.Why 32 and not 1. Shrinking after every large batch would make sustained large traffic realloc from 4 KiB up to 64 KiB per batch. The counter resets to zero on every large batch, so a burst spread over many batches is never interrupted mid-flight; only a session that has genuinely gone quiet pays the regrowth, once.
Changes
src/socket/noise_socket.rs:OUT_BUF_IDLE_CAPACITY/SMALL_BATCHES_BEFORE_SHRINK, two locals tracking whether the buffer is still holding a grown allocation and how long it has been quiet, and the reset after the write. One test.Cost
8.5 vs 8.2 KiB is the point: after the shrink a socket that once sent a large frame costs the same as one that never did.
What the report attributes: still nothing. This buffer remains unreadable from outside the task, so
resource_report()does not name it before or after — the same reason#1235documented it as unexpressible. The change reduces real memory the report never claimed, which means the gap betweentotal_estimated_bytes()andRssAnonnarrows by this much.The side this makes worse: a session that goes quiet for 32 batches and then bursts again re-grows the deque, roughly four doublings of memcpy against a batch that is about to be AEAD-encrypted and pushed through TLS. Sustained large traffic never reaches the threshold, so it never pays this at all.
Checked and not changed
a_batch_never_overshoots_the_byte_ceilingstill passes untouched, and the new test re-asserts the ceiling over the post-shrink burst as well.carry_over, and the waiter bookkeeping. Not touched; the reset is strictly after the write.MAX_BATCH_WIRE_BYTESandMAX_BATCH_FRAMES. Unchanged — this is about what the buffer keeps between batches, not what a batch may hold.Validation
cargo fmt --all --checkcargo test -p whatsapp-rust --lib— 1442 passedcargo clippy --workspace --all-targets -- -D warnings— clean (whatsapp-rust-voip-cliexcluded locally: its build script needsalsa.pc, unavailable here; CI covers it)New behaviour is covered by
a_burst_after_the_buffer_shrinks_still_batches_and_round_trips, which grows the buffer with a 40 KiB frame, quietens past the threshold, then sends a fresh burst and asserts three things the shrink could plausibly have broken: the ceiling still holds, the regrown buffer still coalesces, and every frame decrypts in unbroken counter order across the whole connection.One note on that test: the warm-up sends sequentially rather than through
queue_all. Concurrent sends past the job channel's capacity of 8 do not queue in call order — the first draft did usejoin_alland failed with frames off by one, which is a property of the fixture, not of the sender.Generated by Claude Code