perf(client): send queued acks and receipts as one burst - #1120
Conversation
A batch of one is the overwhelmingly common case, and wrapping its failure in SharedSendFailure buried the transport's own error type one level down: downcast_ref inspects the concrete type rather than walking the chain, so a caller with a custom Transport lost the typed error it used to recover. Sharing only pays for itself when there is more than one waiter.
The noise sender coalesces whatever is queued when it wakes, but both workers awaited each send before reading the next, so neither ever had two frames queued at once. Batching only fired when two different producers happened to interleave, which in the pingpong harness was ~9% of frames. Each worker now drains what is already waiting (try_recv only, never a wait for work that has not arrived), marshals the whole burst synchronously, and hands it over in one go. The socket is resolved once before the burst, so the only await left inside each send is the channel push, which resolves on its first poll: arrival order survives, which the ack worker documents and callers rely on. CapturingMockTransport::sent() now splits writes into frames. Its callers all assume one frame per write and decrypt under the write's index as the counter, which silently stopped holding once batches actually formed; sent_writes() and write_count() expose the raw writes for tests that care about transport behaviour instead.
The send-job channel holds 8, so a burst of 16 fills it and makes unrelated producers wait for a slot: the harness showed 29% fewer writes but 3.7% worse pong latency (paired t = 2.8) at that size. At 4 the write saving is ~16% with latency no worse than main. Raising the channel instead was measured too. It recovers the latency but gives back most of the coalescing (-17.6% writes), because a sender that never has to wait consumes jobs one at a time - the queueing pressure is part of what creates something to coalesce.
📝 WalkthroughWalkthroughClient sending now supports bounded raw-frame bursts. Deferred ACKs and delivery receipts batch queued frames, while marshaling, error propagation, teardown gating, and mock transport assertions are updated for per-frame outcomes. ChangesBatched stanza transmission
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant Client
participant NoiseSocket
participant Transport
Worker->>Client: prepare ACK or receipt frames
Client->>NoiseSocket: send_raw_bytes_burst
NoiseSocket->>Transport: encrypt and send each frame
Transport-->>NoiseSocket: send outcomes
NoiseSocket-->>Client: ordered per-frame results
Client-->>Worker: log individual failures
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/client/messaging.rs | Introduces ordered multi-frame submission and separates node marshalling from asynchronous sending. |
| src/client/node_io.rs | Batches deferred acknowledgements while retaining teardown gating, ordered preparation, tracing, and flush-guard release. |
| src/message/dispatch.rs | Batches queued delivery receipts and reports per-frame send failures under a burst span. |
| src/receipt.rs | Extracts synchronous delivery-receipt eligibility, construction, logging, and marshalling for burst use. |
| src/socket/noise_socket.rs | Preserves an unwrapped transport error for a lone waiter and adds stronger ordering coverage. |
| src/transport.rs | Makes captured sends frame-aware while exposing raw writes separately for transport-level assertions. |
| src/socket/error.rs | Adds a named per-frame encryption-and-send result type. |
| src/client/tests.rs | Adds regression coverage for burst draining, coalescing, teardown, and transport failure paths. |
Sequence Diagram
sequenceDiagram
participant Queue as Ack/Receipt Queue
participant Worker as Persistent Worker
participant Client
participant Noise as NoiseSocket Sender
participant Transport
Worker->>Queue: Receive first item
Worker->>Queue: Drain up to four ready items
Worker->>Client: Prepare frames synchronously
Client->>Noise: Submit frame burst in order
Noise->>Noise: Encrypt and coalesce queued frames
Noise->>Transport: Send combined transport write
Transport-->>Noise: Write result
Noise-->>Client: Return one result per frame
Client-->>Worker: Release flush guards
Reviews (5): Last reviewed commit: "perf(client): reuse the workers' burst b..." | Re-trigger Greptile
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Both workers stopped going through the function that carried their tracing span: receipts through send_delivery_receipt (wa.receipt.send_delivery) and acks through send_ack_for (wa.conn.ack). Since the burst path is the normal one, tracing users lost the span for essentially every live receipt and ack. The receipt span moves to prepare_delivery_receipt, which both paths share. Acks get a wa.conn.ack_burst span reporting the burst size instead of N per-ack spans, applied with instrument() because an EnteredSpan is not Send and cannot be held across the await. Also adds order_survives_a_full_job_channel: the ordering guarantee rests on parked senders being woken in queue order once the job channel fills, which nothing covered.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
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 739-743: Update the body-decryption loop in the relevant test to
assert each decrypted body matches the expected payload for its submission
index, rather than relying only on decrypt_in_place_with_counter succeeding.
Keep the counter-based authentication check, and compare the decrypted contents
against the FIFO-ordered expected payload so reordered jobs fail the test.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3efd053b-4541-44c2-b35a-b7e5d48166be
📒 Files selected for processing (3)
src/client/node_io.rssrc/receipt.rssrc/socket/noise_socket.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cb61a7c40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…rst path Three review findings, all real: send_ack_for returns early on expected_disconnect and errors when not connected; bursting called encode+send directly and skipped both, so a 515 or an intentional disconnect could still write stale acks and hold the outbound flush until its timeout. The queue is still drained, exactly as the one-at-a-time worker did. The receipt span was moved onto the synchronous preparation, which closes it before the await: every receipt would look instant and transport stalls would fall outside it. It goes back on the async single-receipt path, and the worker gets an instrumented burst span like the ack worker already has. order_survives_a_full_job_channel asserted only that each frame decrypts under its position's counter, which reordered jobs would also satisfy since they would be encrypted in the order they woke. It now asserts the payload.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
The gate the burst path applies was an inline condition duplicating what send_ack_for checks, so nothing would fail if a future edit dropped it. It is now a named predicate with a test covering both signals it folds in. Also records why the receipt worker deliberately has no such gate: the single-receipt path never had one either, and adding it for symmetry would start dropping receipts that today still go out.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8610a0839f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…ide the span The two workers allocated batch/frames/guards per burst and handed the frame vector away by value; they now keep all three for the worker's lifetime. Worth it on measurement, not on principle: 173.67 -> 166.83 allocator calls per message (-3.94%, t = -13.0) and -0.92% bytes, harness pingpong at 12k/s. send_raw_bytes_burst takes &mut and always drains, including when no socket is installed. Leaving frames behind would let a caller that forgets to clear resend the same acks on its next burst, which is a duplicate on the wire rather than a wasted allocation; a debug_assert pins the contract at both call sites. The single-frame case skips join_all entirely. The burst result inspection also moves inside the instrumented future. It was running after .instrument(...).await had closed the span, so a failed burst left the span with no error recorded and emitted its warning outside it, unlike the send_ack_for path it replaced.
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
Follow-up to #1119. That PR taught the noise sender to coalesce queued frames into one transport write; this one gives it something to coalesce.
Why #1119 only reached ~9% of frames
Both outbound workers had the same shape:
One job, await it fully, then read the next. So a worker never had two frames queued at once, and the sender's batching only fired when two different producers (an ack, a receipt, a reply) happened to interleave by luck.
Each worker now takes everything already waiting (
try_recvonly - never a wait for work that has not arrived), marshals the whole burst synchronously, and hands it over in one call.Order is preserved, deliberately
The ack worker documents that acks leave in arrival order, and callers rely on it.
send_raw_bytes_burstresolves the noise socket once, before the burst, so the only await left inside each send is the channel push - and a channel with room resolves that on its first poll. Polling the joined sends in order therefore queues them in order.Resolving the socket per send would put a contended
Mutexbetween the futures and let them queue in any order. That is the whole reason the socket lookup is hoisted rather than left where it was.The burst size is measured, not guessed
This is a real tradeoff and the first value tried was wrong. Measured on battery, where CPU noise is ~3.5x higher, so read these as relative to each other rather than as absolute figures:
The send-job channel holds 8. A burst of 16 fills it, so unrelated producers - the reply that actually answers the user - wait for a slot. That is where the latency went.
Raising the channel was tried and rejected: it recovers the latency but gives back most of the coalescing, because a sender that never has to wait consumes jobs one at a time. The queueing pressure is part of what creates something to coalesce in the first place. Capping the burst at 4 keeps half the channel free for everyone else and holds the write saving.
Measurements
Harness
pingpong, 120k messages at 12k/s, againstmain(662bd734, i.e. #1119 already in). Interleaved ABBA/BAAB, pre-built binaries with the sha256 recorded per run, machine on AC. Every run hadlost=0andack=120000.Six pairs against the final tree:
Allocations, measured separately because the counting allocator perturbs the run (four pairs,
MODE=rss, isolating the buffer reuse):On the size of the write saving. An earlier ten-pair round measured -16.3% against an earlier commit of this branch. The difference is the baseline, not the branch:
mainitself measured 3.251 writes/msg in that window and 3.647 in this one, because how much the sender coalesces on its own depends on how the host happens to be scheduling. Both rounds agree on the direction and both are far outside the noise; read the saving as "roughly 11-16% fewer writes, depending on how much main was already coalescing", not as a single number.On CPU. The earlier round on this branch showed no CPU movement (+0.85%, t = 1.1). It moves now, and the buffer reuse is the plausible reason: 4% fewer allocator calls is the kind of thing that shows up as ~2% CPU on an allocation-heavy path. Latency stays flat throughout, which was the constraint that decided the burst size.
Taken together with #1119, writes per message on this workload go from 3.84 (before either PR) to ~3.25 in this window.
Test-side fallout, which is the interesting part
CapturingMockTransport::sent()returned rawsend()payloads, and its 21 call sites all assume one frame per write -frame[3..]decrypts the rest of the buffer, under the write's index as the nonce counter. That silently stopped holding the moment batches actually started forming:duplicate_message_is_acked_with_delivery_receiptfailed with 0 receipts found, because nothing decrypted at all.The fix is at the source rather than at the 21 call sites:
sent()now splits each write into its frames, so "the Nth frame, decrypted under counter N" is true again whether or not a batch formed.sent_writes()andwrite_count()expose the raw writes for tests that want to assert transport-level behaviour.Worth flagging for review: this is the second time a helper that assumed one-frame-per-write has hidden a change in behaviour (the first was
test_concurrent_sends_maintain_orderin #1119).Review findings already folded in
Three came back on the first round and all three were real:
send_ack_forreturns early onexpected_disconnectand errors when not connected; calling encode+send directly bypassed both, so a 515 or an intentional disconnect could still write stale acks and hold the outbound flush until its timeout. The queue is still drained, exactly as the one-at-a-time worker did.wa.receipt.send_deliveryandwa.conn.ack, so tracing users lost the span for essentially every live receipt and ack. My first fix put the receipt span on the synchronous preparation, which closes it before the await - every receipt would look instant and transport stalls would fall outside it. It is now back on the async path, and each worker has an instrumented burst span.order_survives_a_full_job_channelproved less than it claimed. Asserting only that frame N decrypts under counter N is satisfied by reordered jobs too, since they would be encrypted in whatever order they woke. It asserts the payload now..instrument(...).awaithad closed the span, so a failed burst left the span with no error recorded and emitted its warning outside it - unlike thesend_ack_forpath it replaced, which haderr(Debug). The inspection is inside the instrumented future now.Buffer reuse
The workers allocated
batch/frames/guardsper burst and handed the frame vector away by value; all three now live for the worker's lifetime. That is the -3.94% allocator calls above - worth it on measurement rather than on principle.send_raw_bytes_bursttherefore takes&mut Vecand always drains, including when no socket is installed. Leaving frames behind would let a caller that forgets to clear resend the same acks on its next burst - a duplicate on the wire, not a wasted allocation - so adebug_assertpins the contract at both call sites. A burst of one skipsjoin_allentirely.Also in here
A fix for a regression #1119 introduced: when a batch holds exactly one frame - overwhelmingly the common case - its failure was still wrapped in the shared-
Arcerror type, which buried the transport's own error one level down.downcast_refinspects the concrete type rather than walking the chain, so a caller with a customTransportlost the typed error it used to recover. A lone waiter now gets the error untouched; sharing only pays for itself when there is more than one waiter.a_lone_waiter_gets_the_transport_error_untouchedpins it, and fails on the previous behaviour.Verification
cargo fmt --all,cargo clippy --workspace --all-targetswith zero warnings,cargo test -p whatsapp-rust --libgreen (1212 tests).