perf: four more cuts to the per-message allocation count - #1137
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces caller-owned burst result buffers, reusable session chain-key storage, direct JID destination encoding for DSM messages, and streaming reporting-token HMAC calculation. Call sites and tests are updated for the revised APIs and behavior. ChangesBurst result buffering
Chain-key buffer reuse
DSM destination encoding
Streaming reporting tokens
Estimated code review effort: 4 (Complex) | ~45 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 |
|---|---|
| wacore/src/messages.rs | Introduces allocation-free DSM destination encoding and blanket reference implementations that resolve the previously reported generic coercion regressions. |
| wacore/src/reporting_token.rs | Streams ordered reporting-token pieces into the HMAC while retaining concatenated extraction for existing callers. |
| wacore/libsignal/src/protocol/state/session.rs | Reuses uniquely owned, same-length chain-key buffers while preserving allocating fallbacks. |
| src/client/messaging.rs | Changes burst sending to populate and clear a caller-owned reusable results buffer. |
| wacore/src/send/dm.rs | Passes JIDs directly into DSM encoding instead of allocating intermediate strings. |
Sequence Diagram
sequenceDiagram
participant Send as DM send path
participant DSM as MessageUtils
participant Token as Reporting token
participant Signal as Signal session
participant Socket as Burst sender
Send->>DSM: "Encode plaintext with &Jid"
DSM-->>Send: Recipient and own-device bytes
Send->>Token: Collect whitelisted pieces once
Token-->>Send: Streamed HMAC token
Send->>Signal: Encrypt per device
Signal->>Signal: Reuse eligible chain-key buffer
Send->>Socket: Send frames into reusable results buffer
Socket-->>Send: Per-frame results
Reviews (9): Last reviewed commit: "docs(client): mark what the burst out-pa..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/src/reporting_token.rs (1)
828-847: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winReuse the collected pieces instead of parsing and rebuilding them twice.
Line 830 builds
pieces, then Line 847 indirectly callscollect_reporting_token_piecesagain. Nested fields therefore rebuild their owned framing twice, which can negate—or exceed—the allocation savings this PR targets. Pass the first collection into the HMAC helper.Proposed fix
- collect_reporting_token_pieces(encoded_message, REPORTING_FIELDS)?; + let pieces = collect_reporting_token_pieces(encoded_message, REPORTING_FIELDS)?; ... - let token = calculate_reporting_token_streaming(&key, encoded_message, REPORTING_FIELDS)?; + let token = calculate_reporting_token_from_pieces(&key, encoded_message, &pieces)?;fn calculate_reporting_token_from_pieces( reporting_token_key: &[u8; REPORTING_TOKEN_KEY_SIZE], data: &[u8], pieces: &[(u32, Piece)], ) -> Option<[u8; REPORTING_TOKEN_SIZE]> { let mut mac = Hmac::<Sha256>::new_from_slice(reporting_token_key).ok()?; for (_, piece) in pieces { match piece { Piece::Borrowed(range) => mac.update(&data[range.clone()]), Piece::Owned(bytes) => mac.update(bytes), } } let result = mac.finalize().into_bytes(); let mut token = [0u8; REPORTING_TOKEN_SIZE]; token.copy_from_slice(&result[..REPORTING_TOKEN_SIZE]); Some(token) }This defeats the stated allocation-reduction objective.
🤖 Prompt for 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. In `@wacore/src/reporting_token.rs` around lines 828 - 847, Reuse the pieces collected by collect_reporting_token_pieces instead of invoking that function again through calculate_reporting_token_streaming. Update the reporting-token flow and HMAC helper to accept the collected [(u32, Piece)] data, iterate over borrowed and owned pieces, and preserve the existing token output.
🤖 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/client/tests.rs`:
- Around line 3430-3435: Strengthen the results-buffer reuse assertion around
the burst calls by preallocating the caller-owned Vec to the maximum burst size
and recording its as_ptr() before each call. Assert that the pointer remains
identical across calls, rather than only checking results.capacity(), while
preserving the existing out-parameter reuse behavior.
In `@wacore/src/messages.rs`:
- Around line 19-21: Document in the DsmDestination trait that encoded_len()
must exactly match the number of bytes write_into() appends to out, or seal the
trait if custom implementations are not supported and only &str and &Jid are
intended.
In `@wacore/src/send/dm.rs`:
- Around line 181-182: Update the preceding comment in the send flow to state
only that the unused DSM buffer is skipped, removing any claim about avoiding
destination-JID stringification. Delete the duplicate rationale comment adjacent
to the wire-buffer handling, keeping the allocation rationale solely at the
decision point.
---
Outside diff comments:
In `@wacore/src/reporting_token.rs`:
- Around line 828-847: Reuse the pieces collected by
collect_reporting_token_pieces instead of invoking that function again through
calculate_reporting_token_streaming. Update the reporting-token flow and HMAC
helper to accept the collected [(u32, Piece)] data, iterate over borrowed and
owned pieces, and preserve the existing token output.
🪄 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: a4d6277a-71ed-4c65-90d9-17e380c1e7bf
📒 Files selected for processing (8)
src/client/messaging.rssrc/client/node_io.rssrc/client/tests.rssrc/message/dispatch.rswacore/libsignal/src/protocol/state/session.rswacore/src/messages.rswacore/src/reporting_token.rswacore/src/send/dm.rs
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
Merging this PR will regress 0 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | bench_full_token_generation_simple |
30 B | 15 B | ×2 |
| 👁 | Simulation | bench_content_extraction_simple |
10.2 µs | 11.4 µs | -10.25% |
| 👁 | Simulation | bench_reject_prekey_as_signal |
1.1 µs | 1.3 µs | -13.43% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing perf/alloc-round4 (56a02c3) with main (1d38b27)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70a8b690a0
ℹ️ 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".
|
@codspeedbot why the performance regrets |
Bytes is immutable, so assigning a fresh copy_from_slice allocated on every advance, and the ratchet advances three times per message round trip: twice sending, once receiving. After a checkout the record is uniquely owned, since the cache takes it out of its Arc with try_unwrap, so the buffer already there is almost always ours to overwrite. A buffer that is still shared, or one of an unexpected length from an older record, falls back to allocating exactly as before: overwriting a shared buffer would show a holder a key it never asked for, and overwriting a shorter one would leave a key half old and half new.
send_raw_bytes_burst returned a Vec, so it allocated once per burst, and the common burst is a single frame: that allocation was the dominant cost of sending one. Both callers are long-lived worker loops that already reuse their frame and guard buffers, so they reuse this one too. The function clears the buffer up front rather than trusting callers, since a stale result from the previous burst would be reported against the wrong frame.
…first The extractor already stages the whitelisted fields as ranges and sorts them; the Vec existed only to hand the HMAC one slice. Mac::update is associative over its input, so feeding each piece in token order hashes exactly the bytes the concatenation would have held, without building it. extract_reporting_token_content stays for callers that want the bytes. The token goes on the wire and the server verifies it, so a divergence here is a rejected message rather than a slow one. The identity test covers a flat field, a nested one, multibyte content, and a two-field message: that last case is the one that matters, since with a single piece the order cannot be observed and reversing the visit order passes unnoticed.
The DSM field writes the destination's length before its bytes, so the send path rendered the JID into a String purely to measure it and copy it out again. DsmDestination lets a Jid do both without the intermediate: count what a render would write, then write it straight into the wire buffer. The &str impl keeps every existing caller working, and both share one body rather than the DSM format existing in two shapes. The hazard is a counted length that disagrees with the bytes written: the prefix would then point past the payload and the peer would read the next field from the wrong offset. A test pins the two against each other across every server form, agent and device combination, and multibyte user parts.
The streaming reporting token collected its pieces twice: once to test for content and once to hash them. A nested field owns its re-framed bytes, so for any message carrying media or quoted text that materialised those buffers twice and parsed the protobuf twice, making the change slower than the concatenation it replaced. Collected once and reused now, with a test-only tally pinning it: reintroducing the second collection takes the count from 2 to 4. DsmDestination gains a &String impl, because a generic parameter does not deref-coerce the way the old &str argument did, and its contract is now documented on the trait: encoded_len must equal what write_into appends, or the length prefix points past its own payload. The burst buffer test compared capacity, which a fresh Vec of the same capacity satisfies. It compares the allocation itself now, and fails when the callee replaces the buffer with an equal one. Also drops a stale rationale in dm.rs that still described the JID stringification this PR removed.
ceccdef to
8db919f
Compare
Looked into both flagged benchmarks. Neither is a real regression on the DM path this PR targets — one is runner noise, the other is a small, benign shift on a benchmark dominated by code this PR doesn't touch.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/client/tests.rs`:
- Around line 3412-3440: Capture the preallocated results buffer pointer
immediately after Vec::with_capacity(4), before the first send_raw_bytes_burst
call. Assert results.as_ptr() matches that pointer after the single-frame burst
and retain the existing identity assertion after the four-frame burst.
In `@wacore/src/reporting_token.rs`:
- Around line 740-757: Update calculate_reporting_token_over_pieces to return
None immediately when pieces is empty, before initializing or finalizing the
HMAC, while preserving the existing token calculation for non-empty pieces.
Extend a_message_with_nothing_to_report_produces_no_token to directly invoke the
streaming calculation path with the test key and assert it returns None, rather
than only exercising piece collection.
- Around line 815-818: Remove the call-site comment above
collect_reporting_token_pieces in the reporting-token flow, keeping the
rationale only in calculate_reporting_token_over_pieces’ documentation. Leave
the pieces collection and subsequent HMAC behavior unchanged.
🪄 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: b3f599ce-2268-47c0-b5be-dd4ed9f297af
📒 Files selected for processing (8)
src/client/messaging.rssrc/client/node_io.rssrc/client/tests.rssrc/message/dispatch.rswacore/libsignal/src/protocol/state/session.rswacore/src/messages.rswacore/src/reporting_token.rswacore/src/send/dm.rs
The burst test captured the results pointer between the two calls, so a single-frame path that replaced the caller's buffer would have been compared against its own replacement. Captured before the first call now, and asserted after both; mutating that path to swap the buffer fails it. `calculate_reporting_token_over_pieces` hashed whatever it was given, so an empty list would have minted a valid token over no content. Both callers reach it through the collector, which already returns `None` in that case, but a token that says nothing must not depend on a caller remembering to check. The test named for that property only exercised the collector; it now asks the HMAC directly. Also drops a rationale repeated at the call site of the function whose doc comment already carries it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2db2d2a75
ℹ️ 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".
A generic parameter does not deref-coerce, so replacing the `&str` argument silently dropped every caller holding its destination in a wrapper. `&String` was repaired when it was reported; `&Box<str>`, `&Rc<str>`, `&Arc<str>` and `&Cow<'_, str>` had the same problem. A blanket `impl<T: Deref<Target = str>>` would be one line but collides with the `&Jid` implementation: coherence cannot rule out `Jid` gaining that `Deref`, so the compiler treats the two as overlapping. Hence a macro over a named list. The test is a compile-time check as much as a runtime one, which is the point: dropping a wrapper from the list fails the build here rather than in a downstream crate.
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 `@wacore/src/messages.rs`:
- Around line 33-63: Update the destination trait implementations generated by
dsm_destination_via_str to cover mutable references (&mut $ty) for every
documented string wrapper, using the same encoded_len and write_into behavior as
immutable references. In encode_dm_plaintexts, remove the unnecessary Copy bound
so mutable destination call sites remain supported.
🪄 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: 1ef3b595-4882-4adf-a263-db76431424a9
📒 Files selected for processing (1)
wacore/src/messages.rs
`&mut String` coerced to `&str` exactly as `&String` did, so the generic parameter dropped it along with the rest. The macro now emits the mutable form of each wrapper, and `str` itself moved into the list so `&mut str` comes with it. `encode_dm_plaintexts` additionally required `Copy`, which no `&mut` reference satisfies. Nothing needed it: the destination is read through `&self` and the one by-value use is a tail call. The wrapper test goes through `encode_dm_plaintexts` as well as the encoded-content entry point, since only the former carried that bound and a test that never called it left the bound free to come back.
A nested reference such as `&&str` coerced to the old `&str` argument and did not reach any implementation in the list, and no finite list can: there is a depth for every entry it names. The trait is implemented on the owned types instead, with two blanket implementations carrying it through `&T` and `&mut T`. That covers any depth and either mutability, and removes the per-wrapper mutable copies the previous commit added. The earlier note stays true and stays recorded: a blanket over `Deref<Target = str>` on the reference types is what does not work, since coherence cannot rule out `Jid` gaining that `Deref`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2bdf2a58b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8d6fc9bb8
ℹ️ 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".
Four independent cuts to the DM round trip, one commit each, from two independent analyses of the same dhat profile (one Opus, one Codex, neither aware of the other). As in the previous round they overlapped almost not at all: two items come from each.
The frame decoder change those analyses also surfaced went out as #1136, kept separate because its risk is retention rather than correctness and it deserved its own measurement. It has since merged, and this branch is rebased on top of it.
Denominator throughout is 116.74 blocks/msg, the library's share; the raw 123.76 includes 7.02 owned by the harness client.
The cuts
1. The chain key buffer is reused across ratchet advances (3.00/msg).
Bytesis immutable, soexisting.key = Some(Bytes::copy_from_slice(..))allocated on every advance, and the ratchet advances three times per round trip: twice sending, once receiving. After a checkout the record is uniquely owned, since the cache takes it out of itsArcwithtry_unwrap, so the buffer already there is almost always ours to overwrite.Two guards decide when it is not, and both matter: a buffer someone else still holds would show that holder a key it never asked for, and a buffer of the wrong length (a record from an older format) would end up half old and half new. Either case falls back to allocating, which is exactly the previous behaviour.
2. The burst callers own their results buffer (1.43/msg).
send_raw_bytes_burstreturned aVec, so it allocated once per burst, and the common burst is a single frame: that allocation was the dominant cost of sending one. Both callers are long-lived worker loops that already reuse their frame and guard buffers. The function clears the buffer itself rather than trusting callers, since a stale result from the previous burst would be reported against the wrong frame.3. The reporting token hashes its pieces instead of concatenating them (1.00/msg). The extractor already stages the whitelisted fields as ranges and sorts them; the
Vecexisted only to hand the HMAC one slice.Mac::updateis associative over its input, so feeding each piece in token order hashes exactly the bytes the concatenation would have held.extract_reporting_token_contentstays for callers that want the bytes.4. The JID names itself into the DSM field (1.00/msg). That field writes the destination's length before its bytes, so the send path rendered the JID into a
Stringpurely to measure it and copy it out again.DsmDestinationlets aJiddo both without the intermediate, and both share one body rather than the DSM format existing in two shapes.Replacing a
&strparameter with a generic one costs deref coercion, which took four rounds of review to get right and is written up under the findings below.Measurements
Harness
pingpong, 120k messages at 12k/s,MODE=rss, three interleaved ABBA/BAAB pairs againstmain(d2a3a16a), pre-built binaries with the sha256 recorded per run. Every run hadlost=0andack=120000.6.53 allocations per message removed, against 6.43 predicted by summing the four items from the profile. One baseline run is excluded: its
wcharcame in at 114 MB against ~81 MB for every other run, which is host I/O in that window rather than branch behaviour. Including it gives -5.76%, the same story with a worse denominator.No CPU claim. These are small allocations served by a warm allocator; earlier profiling on this workload put malloc+free at ~2.8% with the bottleneck in curve25519. Worth doing for allocator pressure, not for a throughput promise.
What each change risks, and what pins it
The two items that touch wire or crypto state carry the real risk, so they carry the real tests.
The chain key is key material. Three mutations, all killed: removing the shared-buffer check (the uniquely-owned test fails, since the buffer is silently replaced instead of reused), removing the length check (a 16-byte buffer gets overwritten with 32 bytes of key), and the absent-buffer path.
The DSM destination writes a length prefix ahead of its payload. A counted length that disagrees with the bytes written would point past the payload, and the peer would read the next field from the wrong offset. A test pins the count against the render across every server form, agent and device combination, and multibyte user parts; mutating the counter by one byte fails it.
The reporting token goes on the wire and the server verifies it. The identity test covers a flat field, a nested one, multibyte content, and a two-field message. That last case is the one that matters and it was added after a mutation survived: with a single piece the visit order cannot be observed, so reversing it passed unnoticed. With two fields, reversing the order now fails the test.
The burst buffer is cleared by the callee; removing that clear leaks the previous burst's results into the next one, which the test catches as 5 results where 4 were sent.
Deliberately left out
Caching the own-identity
Arc<str>(would be 1.00/msg). The identity comes from the persistence manager's device snapshot and changes on a re-pair. A parallel cache that falls out of sync writes themessageSecretunder the wrong identity, and reading those secrets back (msmsg replies) then fails. The safe shape is to hold theArc<str>inside the snapshot itself, which is a much larger change than the item suggests. Not worth it for one allocation.A peer-JID memo for the same site was speculative even in the analysis that proposed it: it puts a lock on the hot path to save an allocation, which may cost more wall clock than it saves.
Reusing the session record's
Arcacross checkout and commit (3.00/msg). It means changingSessionCheckout::committo take anArc<SessionRecord>, a cross-crate trait signature on the encryption path, where a mistake is a ratchet desync rather than a crash. Not while cheaper items remain.Verification
cargo fmt --all,cargo clippy --workspace --all-targetswith zero warnings,RUSTDOCFLAGS="-D warnings" cargo docclean, and green suites:wacore1268,whatsapp-rust1230,wacore-libsignal199.Review findings folded in after opening
Two tests of mine proved nothing, and only mutation said so. The burst test captured the results pointer between the two calls, so a single-frame path that replaced the caller's buffer would have been compared against its own replacement; it is captured before the first call now. And
calculate_reporting_token_over_pieceshashed whatever it was handed, so an empty list would have minted a valid token over no content -- unreachable today, since both callers pass through the collector, but the test named for that property only exercised the collector. It asks the HMAC directly now.A regression this PR introduced: the streaming path collected the pieces once to test for content and again to hash them, so a nested message allocated four times where it used to allocate three. Collected once now, pinned by a call tally that reads 2 for a nested message and 4 if the second collection returns.
Replacing
&strwith a generic parameter broke deref coercion, in four widening steps.&Stringfirst, then&Box<str>/&Rc<str>/&Arc<str>/&Cow<str>, then the mutable forms plus aCopybound that no&mutsatisfies, and finally nested references such as&&str-- which no finite list of implementations can cover, since there is always one more level than it names.The shape it landed on: implement the trait on the owned types, and let two blanket implementations carry it through
&Tand&mut T. That covers any depth and either mutability at once.Recorded because it was proposed twice and does not work: a blanket over
Deref<Target = str>collides three ways simultaneously (E0119against&_,&mut _, andJid) --Jidbecause coherence cannot rule out it gaining thatDeref, and the reference cases because&strderefs tostrtoo.Declined: keeping a
&strentry point alongside the generic one so that foreign string newtypes keep coercing. It restores the two callable shapes this item set out to remove, with the&strentry point existing purely to preserve a coercion, while the only in-tree caller passes a&Jid. A newtype has&*wrapper, or can implement the public trait; both are documented on it now.CodSpeed flags two regressions; neither warrants a change.
bench_reject_prekey_as_signalran on a different CI runner (Simulation mode derives its cache model from the physical CPU) and its measured body isSignalMessage::try_fromplus an error assertion, which touches nothing here.bench_content_extraction_simpleis ~1.2us on a benchmark that is ~72%message_to_vec, code this PR does not touch. The allocation win the change was made for shows up where it should:bench_full_token_generation_simplegoes 30 B -> 15 B.