perf: five cuts to the per-message allocation count - #1131
Merged
Conversation
The id a send is known by existed three times: a copy for the returned SendResult, a copy kept for the post-send messageSecret write, and a third read back out of the finished stanza's attributes because the original had already been moved into a branch builder. Lend it instead. The outer frame keeps the only String and hands the branch requests, the phash ack-waiter and the secret persistence a &str, so downstream callers that already own an id (peer PDO, app state key requests, group member labels) stop copying it as well. The two properties that made the third copy look necessary are kept: the ack-waiter is still registered before the stanza reaches the socket, and the stanza is still the authority on the id it carries -- now asserted in debug builds rather than re-read on every send.
A DM already sizes one participant vector for its whole fan-out, then threw two more away filling it: each half built a node list and a list of the devices it encrypted for, only to be moved into that vector and dropped. Two halves per message, so four vectors per message. The device list exists for the group path, which compares it against the distribution list to tell a partial SKDM from a complete one. A DM never asks that, so it gets its own entry point that appends straight into the caller's buffer and reports only the two batch flags. The shared EncryptResult and the raw form the VoIP offer uses are untouched.
Every caller that needed the per-device session locks resolved the whole handle set into a vector and then walked it locking each one. The handle vector existed only to be walked: nothing reads a mutex it does not immediately lock. Locking as each handle is resolved drops that vector and leaves the guard vector, which has to stay -- it is what holds the locks open for the caller's scope. Acquisition order is unchanged and stays the caller-supplied (sorted) order, which is the only thing keeping two overlapping sends from deadlocking; it now has a test that fails if the loop ever runs the other way. The handle-only helper survives for tests that inspect the mutexes without taking them.
A one-device fan-out named the same device twice: the session phase built a scratch address to ask the store whether a session existed, dropped it, and the encrypt built a second one for the address it had just been told about. The plan now carries that buffer forward, so the single-device branch rewrites it instead of allocating. It is a rewrite, not a reuse of the contents: on a cold PN device the session phase leaves the buffer holding the PN name while the session it just created lives under the device's LID address, so the encrypt must restate which address it wants. The multi-device branch is unchanged -- its per-job addresses outlive the call in spawned tasks -- and a plan built by assume_ready carries no buffer and keeps building its own.
Field extraction copied every flat field into a vector of its own, put those in a heap-allocated staging list, then copied them a second time into the result. Three allocations to answer a question whose answer, for a plain text message, is one slice of the input. A flat field is now recorded as a range into the input and copied straight into the result; only a nested field, which is re-framed under a fresh tag and length, still owns bytes. The staging list is inline for the field counts a real message has and spills only past four. The token's bytes are a wire contract, so what did not change: the sort is still by ascending field number and still stable, so repeats of one field keep their wire order, and the concatenation is byte for byte the same.
Every protocol address was a 64-byte heap allocation for a string that fits in a cache line: a real one is `"5511987650001:5@c.us.0"`, 22 bytes. The send path builds several per message -- one to ask the store about a session, one per encrypt, one per lock key -- and each of them paid for a buffer it then filled with twenty-odd characters. The buffer is now inline up to 47 bytes and spills to a String beyond that, so an address that outgrows it is no worse off than every address used to be. Which arm holds the characters is deliberately not part of the value: equality, ordering and hashing all read the string, because the type is a HashMap key in the session cache and an inline key must find a spilled entry. Two things follow from the buffer no longer being a String. The address format has one writer serving both a plain String and this buffer, so it takes a small sink trait rather than existing twice. And building an address from a JID writes into it directly instead of formatting a String to copy in and drop -- which is what removes the allocation from the paths that build one per message.
📝 WalkthroughWalkthroughThis PR adds inline/heap-backed protocol-address storage, borrows send request IDs, centralizes session guards, writes DM fan-out nodes into existing buffers, reduces reporting-token allocations, and updates dependency declarations and call sites. ChangesCore send and buffer changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SendPipeline
participant SessionGuards
participant DmEncryption
participant Stanza
Client->>SendPipeline: submit borrowed request_id
SendPipeline->>SessionGuards: acquire ordered session guards
SessionGuards-->>SendPipeline: return guards
SendPipeline->>DmEncryption: encrypt devices into participant buffer
DmEncryption->>Stanza: append participant nodes
SendPipeline-->>Client: return send result
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/send/mod.rs | Borrows request IDs across the send pipeline, replaces staged session mutex resolution with ordered guard acquisition, and adds focused invariant tests. |
| wacore/src/send/encrypt.rs | Adds direct append-based device encryption to avoid temporary fan-out vectors while retaining existing encryption result paths. |
| wacore/src/send/dm.rs | Builds DM participant nodes directly in the final stanza allocation. |
| wacore/libsignal/src/core/address.rs | Introduces inline-or-spilled protocol-address storage with content-based equality, ordering, hashing, and reset semantics. |
| wacore/src/types/jid.rs | Generalizes Signal-address rendering to reusable sinks and supports explicit protocol-address rewrites. |
| wacore/src/reporting_token.rs | Uses borrowed input ranges and small-vector storage to reduce reporting-token extraction allocations without changing output ordering. |
| wacore/src/store/signal_cache.rs | Adapts Signal cache operations to the new protocol-address representation while retaining textual cache keys. |
| src/voip/facade.rs | Uses the shared ordered session-guard helper in VoIP encryption paths. |
Sequence Diagram
sequenceDiagram
participant Caller
participant Pipeline as Send pipeline
participant Locks as Session locks
participant Signal
participant Store
participant Socket
Caller->>Pipeline: send(message, request_id)
Pipeline->>Locks: acquire sorted device locks
Pipeline->>Signal: encrypt using rewritten ProtocolAddress
Signal-->>Pipeline: append encrypted nodes into stanza
Pipeline->>Store: persist Signal state before wire
Pipeline->>Pipeline: register phash waiter by request_id
Pipeline->>Socket: send stanza
Socket-->>Pipeline: send completed
Pipeline->>Store: persist message secret by request_id
Pipeline-->>Caller: SendResult with same request_id
Reviews (9): Last reviewed commit: "test: apply the ordering fix to the DM s..." | Re-trigger Greptile
Rustdoc rejects a public item documenting itself through a private one, so the inline capacity is stated as a number instead of a link.
Both members pinned "1.15" independently, which is how the two drift. The version comment above the workspace list is gone because cargo sort does not preserve leading comments in the table it rewrites.
Mechanical reordering only; no dependency added, removed or repinned.
📦 Binary size report
.text per crate
Top movers (cargo-bloat attribution)
Baseline: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3967a2774d
ℹ️ 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".
Resetting an address only rewinds the length, so the tail of an inline buffer still holds the bytes of whoever it named before. The derived Debug printed the whole array, which means any log line formatting an address, or an error embedding one such as SessionNotFound, could show an unrelated peer's JID and send someone chasing the wrong session.
Contributor
Merging this PR will improve performance by ×4.6
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | bench_content_extraction_simple |
173 B | 30 B | ×5.8 |
| ⚡ | Memory | bench_full_token_generation_simple |
173 B | 30 B | ×5.8 |
| ⚡ | Memory | bench_content_extraction_extended |
471 B | 129 B | ×3.7 |
| ⚡ | Memory | bench_full_token_generation_extended |
471 B | 129 B | ×3.7 |
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-round3 (647d57d) with main (f6a20a3)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/send/mod.rs`:
- Around line 2481-2524: Update session_guards_for to debug-assert that each jid
is strictly greater than the preceding jid according to cmp_for_lock_order
before acquiring its lock, catching unsorted or duplicate keys without changing
release behavior. Track the previous jid while iterating, preserve the existing
ordered locking and guard collection, and leave session_mutexes_for unchanged.
In `@wacore/libsignal/src/core/address.rs`:
- Around line 476-495: Extend the boundary tests around
`the_inline_boundary_holds_on_both_sides` with a multi-digit device ID case
using the suggested name length so the suffix causes the spill. Assert the
buffer is not inline and that both `name()` and `as_str()` preserve the original
name and complete `.123` suffix.
- Around line 329-337: Update ProtocolAddress::empty so its returned value
remains distinct under Hash, PartialEq, and Ord for different device IDs: either
initialize the buffer with the device-id suffix immediately, or restrict this
constructor to internal use until reset_with is required. Preserve the reusable
empty-buffer behavior only if it cannot expose equal as_str() values for
different devices.
In `@wacore/src/reporting_token.rs`:
- Around line 1807-1843: Update the test
the_result_is_allocated_once_for_the_exact_length_it_holds so it no longer
relies on exact Vec capacity or allocator growth behavior. Replace the
capacity-equals-length assertion and related comment with a portable assertion
of the staging allocation contract, or use an explicit allocation-counting setup
that verifies a single allocation without inspecting capacity rounding.
In `@wacore/src/send/encrypt.rs`:
- Around line 374-427: Leave the current DM behavior unchanged; no code change
is required for this non-blocking observation. If addressing it later, update
the DM callers of encrypt_for_devices_into to consume
EncryptFanoutSummary.had_unregistered_device and perform the same device-list
invalidation used by the group path.
- Around line 846-857: The address reuse around encryption_jid should not be
threaded through SessionPlan. Remove the reusable_addr plumbing and construct
the protocol address directly via encryption_jid.to_protocol_address(), keeping
SessionPlan focused on session data; only retain reuse if a benchmark
demonstrates a throughput benefit.
In `@wacore/src/send/tests.rs`:
- Around line 4444-4466: Update the ordering assertions in
many_devices_append_in_order_after_the_existing_content and
a_dm_stanza_carries_both_halves_in_one_participants_node to avoid requiring
device order within each fan-out half. Preserve and assert that the first half’s
members appear before the second half’s members, while comparing membership
within each half without sequence ordering.
🪄 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: c1e01b86-01aa-4fae-81d2-461e3fe03be6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
Cargo.tomlsrc/client/app_state.rssrc/client/context_impl.rssrc/client/messaging.rssrc/client/tests.rssrc/features/groups.rssrc/features/signal.rssrc/message/special.rssrc/pdo.rssrc/send/mod.rssrc/signal_flush.rssrc/store/signal.rssrc/store/signal_adapter.rssrc/voip/facade.rstests/bench-integration/Cargo.tomltests/e2e/Cargo.tomltests/signal_durability_sqlite.rstransports/tokio-transport/Cargo.tomlwacore/Cargo.tomlwacore/benches/send_receive_benchmark.rswacore/binary/Cargo.tomlwacore/libsignal/benches/libsignal_benchmark.rswacore/libsignal/src/core/address.rswacore/libsignal/src/core/mod.rswacore/libsignal/src/protocol/mod.rswacore/libsignal/src/protocol/session_cipher.rswacore/libsignal/src/protocol/storage/traits_hook_tests.rswacore/libsignal/tests/counter_lease.rswacore/libsignal/tests/session_divergence.rswacore/src/reporting_token.rswacore/src/send/dm.rswacore/src/send/encrypt.rswacore/src/send/peer.rswacore/src/send/tests.rswacore/src/store/signal_cache.rswacore/src/store/signal_cache_durability_chaos.rswacore/src/types/jid.rs
Comment on lines
+2481
to
2524
| /// Take every per-device session lock, in `jids` order. | ||
| /// | ||
| /// INVARIANT: acquisition order IS `jids` order, and callers pass keys from | ||
| /// [`Self::build_session_lock_keys`], which sorts them. That single order is | ||
| /// what keeps two sends overlapping on a device from deadlocking, so a | ||
| /// change here has to preserve it. | ||
| /// | ||
| /// Each mutex is locked as it is resolved rather than resolving the whole | ||
| /// set first: the handles exist only to be locked, so the vector holding | ||
| /// them was pure staging. The guards themselves must still be collected — | ||
| /// they are what keeps the locks held for the caller's scope. | ||
| pub(crate) async fn session_guards_for( | ||
| &self, | ||
| jids: &[Jid], | ||
| ) -> Vec<async_lock::MutexGuardArc<()>> { | ||
| let mut guards = Vec::with_capacity(jids.len()); | ||
| // A `ProtocolAddress` IS the "{name}.0" string the lock map is keyed by, | ||
| // and it holds it inline, so the whole loop names its keys without | ||
| // allocating a formatting buffer. | ||
| let mut addr = wacore::types::jid::make_reusable_protocol_address(); | ||
| for jid in jids { | ||
| jid.reset_protocol_address(&mut addr); | ||
| let mutex = self.session_lock_for(addr.as_str()).await; | ||
| guards.push(mutex.lock_arc().await); | ||
| } | ||
| guards | ||
| } | ||
|
|
||
| /// The mutexes [`Self::session_guards_for`] would take, without taking | ||
| /// them. Only tests need this: production code always wants the guards, and | ||
| /// resolving handles it does not lock is what this commit removed. | ||
| #[cfg(test)] | ||
| pub(crate) async fn session_mutexes_for( | ||
| &self, | ||
| jids: &[Jid], | ||
| ) -> Vec<std::sync::Arc<async_lock::Mutex<()>>> { | ||
| let mut mutexes = Vec::with_capacity(jids.len()); | ||
| let mut buf = wacore::types::jid::make_address_buffer(); | ||
| let mut addr = wacore::types::jid::make_reusable_protocol_address(); | ||
| for jid in jids { | ||
| wacore::types::jid::write_protocol_address_to(jid, &mut buf); | ||
| mutexes.push(self.session_lock_for(&buf).await); | ||
| jid.reset_protocol_address(&mut addr); | ||
| mutexes.push(self.session_lock_for(addr.as_str()).await); | ||
| } | ||
| mutexes | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Lock this in: session_guards_for trusts callers not to hand it duplicate keys.
Look, the doc comment is crystal clear that jids must already be sorted and deduped by build_session_lock_keys, and every caller today does exactly that. That's great discipline. But if someone six months from now wires up a new send path and skips build_session_lock_keys, a duplicate jid in the list means mutex.lock_arc().await blocks on a lock this same call already holds — a silent, hard-to-diagnose self-deadlock, not a panic that fails fast. I'd rather this fail loud in debug builds than hang in someone's production incident. A cheap debug_assert! on strictly-increasing order (per cmp_for_lock_order) would close that gap for free.
🛡️ Proposed defensive assertion
pub(crate) async fn session_guards_for(
&self,
jids: &[Jid],
) -> Vec<async_lock::MutexGuardArc<()>> {
let mut guards = Vec::with_capacity(jids.len());
+ debug_assert!(
+ jids.windows(2)
+ .all(|w| wacore::types::jid::cmp_for_lock_order(&w[0], &w[1]).is_lt()),
+ "session_guards_for requires sorted, deduped keys (use build_session_lock_keys)"
+ );
// A `ProtocolAddress` IS the "{name}.0" string the lock map is keyed by,
// and it holds it inline, so the whole loop names its keys without
// allocating a formatting buffer.
let mut addr = wacore::types::jid::make_reusable_protocol_address();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Take every per-device session lock, in `jids` order. | |
| /// | |
| /// INVARIANT: acquisition order IS `jids` order, and callers pass keys from | |
| /// [`Self::build_session_lock_keys`], which sorts them. That single order is | |
| /// what keeps two sends overlapping on a device from deadlocking, so a | |
| /// change here has to preserve it. | |
| /// | |
| /// Each mutex is locked as it is resolved rather than resolving the whole | |
| /// set first: the handles exist only to be locked, so the vector holding | |
| /// them was pure staging. The guards themselves must still be collected — | |
| /// they are what keeps the locks held for the caller's scope. | |
| pub(crate) async fn session_guards_for( | |
| &self, | |
| jids: &[Jid], | |
| ) -> Vec<async_lock::MutexGuardArc<()>> { | |
| let mut guards = Vec::with_capacity(jids.len()); | |
| // A `ProtocolAddress` IS the "{name}.0" string the lock map is keyed by, | |
| // and it holds it inline, so the whole loop names its keys without | |
| // allocating a formatting buffer. | |
| let mut addr = wacore::types::jid::make_reusable_protocol_address(); | |
| for jid in jids { | |
| jid.reset_protocol_address(&mut addr); | |
| let mutex = self.session_lock_for(addr.as_str()).await; | |
| guards.push(mutex.lock_arc().await); | |
| } | |
| guards | |
| } | |
| /// The mutexes [`Self::session_guards_for`] would take, without taking | |
| /// them. Only tests need this: production code always wants the guards, and | |
| /// resolving handles it does not lock is what this commit removed. | |
| #[cfg(test)] | |
| pub(crate) async fn session_mutexes_for( | |
| &self, | |
| jids: &[Jid], | |
| ) -> Vec<std::sync::Arc<async_lock::Mutex<()>>> { | |
| let mut mutexes = Vec::with_capacity(jids.len()); | |
| let mut buf = wacore::types::jid::make_address_buffer(); | |
| let mut addr = wacore::types::jid::make_reusable_protocol_address(); | |
| for jid in jids { | |
| wacore::types::jid::write_protocol_address_to(jid, &mut buf); | |
| mutexes.push(self.session_lock_for(&buf).await); | |
| jid.reset_protocol_address(&mut addr); | |
| mutexes.push(self.session_lock_for(addr.as_str()).await); | |
| } | |
| mutexes | |
| } | |
| /// Take every per-device session lock, in `jids` order. | |
| /// | |
| /// INVARIANT: acquisition order IS `jids` order, and callers pass keys from | |
| /// [`Self::build_session_lock_keys`], which sorts them. That single order is | |
| /// what keeps two sends overlapping on a device from deadlocking, so a | |
| /// change here has to preserve it. | |
| /// | |
| /// Each mutex is locked as it is resolved rather than resolving the whole | |
| /// set first: the handles exist only to be locked, so the vector holding | |
| /// them was pure staging. The guards themselves must still be collected — | |
| /// they are what keeps the locks held for the caller's scope. | |
| pub(crate) async fn session_guards_for( | |
| &self, | |
| jids: &[Jid], | |
| ) -> Vec<async_lock::MutexGuardArc<()>> { | |
| let mut guards = Vec::with_capacity(jids.len()); | |
| debug_assert!( | |
| jids.windows(2) | |
| .all(|w| wacore::types::jid::cmp_for_lock_order(&w[0], &w[1]).is_lt()), | |
| "session_guards_for requires sorted, deduped keys (use build_session_lock_keys)" | |
| ); | |
| // A `ProtocolAddress` IS the "{name}.0" string the lock map is keyed by, | |
| // and it holds it inline, so the whole loop names its keys without | |
| // allocating a formatting buffer. | |
| let mut addr = wacore::types::jid::make_reusable_protocol_address(); | |
| for jid in jids { | |
| jid.reset_protocol_address(&mut addr); | |
| let mutex = self.session_lock_for(addr.as_str()).await; | |
| guards.push(mutex.lock_arc().await); | |
| } | |
| guards | |
| } | |
| /// The mutexes [`Self::session_guards_for`] would take, without taking | |
| /// them. Only tests need this: production code always wants the guards, and | |
| /// resolving handles it does not lock is what this commit removed. | |
| #[cfg(test)] | |
| pub(crate) async fn session_mutexes_for( | |
| &self, | |
| jids: &[Jid], | |
| ) -> Vec<std::sync::Arc<async_lock::Mutex<()>>> { | |
| let mut mutexes = Vec::with_capacity(jids.len()); | |
| let mut addr = wacore::types::jid::make_reusable_protocol_address(); | |
| for jid in jids { | |
| jid.reset_protocol_address(&mut addr); | |
| mutexes.push(self.session_lock_for(addr.as_str()).await); | |
| } | |
| mutexes | |
| } |
🤖 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 `@src/send/mod.rs` around lines 2481 - 2524, Update session_guards_for to
debug-assert that each jid is strictly greater than the preceding jid according
to cmp_for_lock_order before acquiring its lock, catching unsorted or duplicate
keys without changing release behavior. Track the previous jid while iterating,
preserve the existing ordered locking and guard collection, and leave
session_mutexes_for unchanged.
An unnamed ProtocolAddress carried no device in its rendered form, so empty(1) and empty(2) hashed and compared alike and would have collided as session-cache keys. The suffix is now written up front. The reporting-token test asserted capacity() == len(), which an allocator is free to break by rounding a request up. The property worth pinning is that the reservation matches the bytes written, so that is now a debug_assert inside the extractor and every test that calls it exercises it. The fan-out sink tests asserted an order that the production code says it does not guarantee: within one half the devices drain from a FuturesUnordered in completion order. They passed only because the current-thread runtime never let those futures pend. They now assert what is guaranteed, that each half lands entirely after the previous one.
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/send/tests.rs`:
- Around line 4466-4479: Add an assertion on written.len() before split_at in
the participant ordering test, verifying it equals first.len() + second.len().
Keep the existing half splitting and BTreeSet content assertions 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: 8a2121c0-bdd8-4093-8521-b061ddaaa600
📒 Files selected for processing (3)
wacore/libsignal/src/core/address.rswacore/src/reporting_token.rswacore/src/send/tests.rs
Comparing each half as a set was the fix for asserting an order the fan-out does not guarantee, but a set also hides a duplicate: a device written twice while another was dropped would still match. The count is also what makes the split into halves mean anything.
session_guards_for documents that its keys arrive sorted and deduped, and every caller honours that, but a future path that forgets would await a lock the same call already holds: a send that never returns rather than a panic. A debug_assert makes it fail loudly instead. The inline boundary was only covered with device 0, which writes its suffix in one push. A multi-digit id goes through write!, so the spill can land between the dot and the digits.
Measured, and it does not pay. Isolating this commit against the rest of the branch gives 128.70 allocations per message without it and 129.86 with it: no saving, and not distinguishable from noise either way (t = 0.90). The reason is the inline address later in this branch. When this was written, building a ProtocolAddress meant a 64-byte heap allocation, so handing the session phase's buffer to the encrypt saved one. Now the buffer lives in the value, so the second construction allocates nothing and the only thing threading an Option<ProtocolAddress> through SessionPlan buys is 72 bytes of state and a rewrite that has to be exactly right. The tests stay. They pin that a cold PN device the session phase upgraded to LID encrypts against the LID session, which is a property of the send path rather than of the optimisation, and they pass unchanged without it.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wacore/src/send/tests.rs (1)
4444-4464: 🎯 Functional Correctness | 🟡 MinorKeep fan-out tests deterministic without requiring completion order.
Both assertions assume input order within a fan-out half, but the implementation drains concurrent work in completion order. Preserve and verify the half boundary, while comparing membership inside each half.
wacore/src/send/tests.rs#L4444-L4464: split the written participants atfirst.len(), compare the first half as a set or sorted list, and retain exact ordering for the second half.wacore/src/send/tests.rs#L4512-L4584: compare recipient devices without sequence ordering, then assert the own-device suffix remains after them.🤖 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/send/tests.rs` around lines 4444 - 4464, The fan-out tests incorrectly require completion order within concurrent batches. In wacore/src/send/tests.rs lines 4444-4464, update many_devices_append_in_order_after_the_existing_content to split written participants at first.len(), compare the first half as an unordered set or sorted list, and retain exact ordering for the second half; in lines 4512-4584, compare recipient devices without sequence ordering while still asserting the own-device suffix follows all recipient devices.
🤖 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.
Duplicate comments:
In `@wacore/src/send/tests.rs`:
- Around line 4444-4464: The fan-out tests incorrectly require completion order
within concurrent batches. In wacore/src/send/tests.rs lines 4444-4464, update
many_devices_append_in_order_after_the_existing_content to split written
participants at first.len(), compare the first half as an unordered set or
sorted list, and retain exact ordering for the second half; in lines 4512-4584,
compare recipient devices without sequence ordering while still asserting the
own-device suffix follows all recipient devices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: af3b9c15-27af-4572-8ce6-84118d5a8af6
📒 Files selected for processing (2)
wacore/src/send/encrypt.rswacore/src/send/tests.rs
The first pass only fixed the sink tests. This one asserted the same unguaranteed order one level up: within the recipient half the devices drain from a FuturesUnordered, so which lands first is not promised. The boundary is, because the two halves are sequential awaits, and swapping them now fails the test.
jlucaso1
changed the title
3 tasks
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Five independent cuts to the DM round trip, one commit each. A sixth was implemented, measured, and reverted inside this PR; that is explained below because the reason is more useful than the change would have been. They come from two independent analyses of the same dhat profile (one Opus, one Codex, neither aware of the other), and the split is worth noting: apart from the
ProtocolAddressreuse, they found different things. Every per-message figure below was confirmed against the profile before implementing.Denominator throughout is 145.84 blocks/msg, the library's share; the raw 152.86 includes 7.02 owned by the harness client.
The cuts
1. One
request_idper send. The same id existed three times: cloned intoSendResult, cloned again for the message-secret write, and re-read from the stanza attribute into a freshString.SendPipelineOptions.request_idis nowOption<&str>, so the outer frame holds the onlyStringand lends it out. Four off-hot-path callers stopped cloning too.Both original invariants survive: the phash waiter is still registered before
send_node(a fast link can ACK while it is still returning), and the stanza remains the authority on its own id, now enforced with adebug_assert_eq!instead of an owned re-read.2. The DM fan-out writes into the stanza it is filling.
prepare_dm_stanzaalready sized its final vector, but each of the two fan-outs per message built aVec<Node>and aVec<Jid>first. A newencrypt_for_devices_intoappends straight into that vector.EncryptResult(which groups need for the SKDM partial-distribution check) andEncryptForDevicesRaw(VoIP) are untouched.3. Lock each session as its mutex is resolved.
session_mutexes_formaterialised aVec<Arc<Mutex<()>>>and then a second vector of guards. Only the first is removable, and acquisition order is unchanged, which matters because that order is what prevents deadlock: a test now fails if the loop reverses.4. (reverted) Carry the session phase's address into the encrypt. Implemented, then measured against the rest of the branch and dropped: 128.70 allocations per message without it, 129.86 with it, i.e. no saving and not distinguishable from noise (t = 0.90).
The reason is item 6, further down. When this was written, building a
ProtocolAddressmeant a 64-byte heap allocation, so handing the session phase's buffer to the encrypt saved one. Once the buffer moved inline, the second construction allocates nothing, and all that threading anOption<ProtocolAddress>throughSessionPlanbuys is 72 bytes of state and a rewrite that has to be exactly right.Its tests stayed. They pin that a cold PN device the session phase upgraded to LID encrypts against the LID session, which is a property of the send path rather than of the optimisation, and they pass unchanged without it. That case is also what caught a real bug while the optimisation still existed: the obvious version reuses the buffer's contents, which on that upgrade path leaves the PN name in front of a session that now lives under the LID address.
5. Copy each whitelisted reporting-token field once. The extractor staged a
Vec<(u32, Vec<u8>)>plus ato_vec()per flat field before sorting by field number. Now aSmallVec<[(u32, Piece); 4]>wherePieceis either aRangeinto the input or an owned buffer for nested fields. Three allocations become one, and the token bytes are unchanged: the existing golden tests pass untouched.6. Keep a protocol address in the value itself.
ProtocolAddresswas{ String, name_len, device_id }with a 64-byte heap allocation for a name of ~20-32 characters. It is now inline up to 47 bytes, spilling to aStringbeyond that.Eq/Ord/Hashreadas_str()only, so an inline key finds a spilled entry in the session cache. This is the property that makes the whole change safe rather than a silent cache-miss generator, and it is tested through a realHashMap.Recorded so nobody re-derives it:
compact_strdoes not work here. Its inline limit is 24 bytes and a PN address ("5511987650001:5@s.whatsapp.net.0") is 32.ProtocolAddressgrows 40 -> 72 bytes. Thesend_futures_stay_smallgate (<=192 B) still passes and no other size assertion in the tree touches it.Measurements
Harness
pingpong, 120k messages at 12k/s,MODE=rss, three interleaved ABBA/BAAB pairs againstmain, pre-built binaries with the sha256 recorded per run. Every run hadlost=0andack=120000.17.16 allocations per message removed. One baseline run is excluded: its
wcharcame in at 115 MB against ~82 MB for every other run, which is host I/O during that window rather than branch behaviour. With it included the figures are -12.47% and -6.49%, the same story with a worse denominator.Worth noting against the earlier revision of this PR: with the reverted item still in, the same measurement gave 17.26 allocations per message. Removing it changed the total by 0.10, which is the cleanest evidence that it was not contributing.
The byte figure is the largest of this campaign, and item 6 is doing that work: the other cuts remove small allocations, while the inline address removes a 64-byte heap block per construction.
No CPU claim. The host had competing load during these runs, so any timing from them is worthless; allocation counts survive that because they are deterministic. Earlier profiling on this workload put malloc+free at ~2.8% with the bottleneck in curve25519, so expect little to no throughput change.
Testing
42 mutants killed across the items. The ones worth naming, because they are the failure modes that would not show up as a crash:
a_cold_pn_device_upgraded_to_lid_encrypts_against_the_new_lid_session(the bug the reverted item would have shipped)Hashreads the representation instead of the contentsthe_same_characters_compare_and_hash_alike_from_either_representationTwo of those were re-run independently on this branch after the rebase, and both still fail under mutation, so the coverage is not accidental.
Worth recording that two tests failed to kill their mutant on the first attempt and had to be rewritten: the
Piece::lentest landed on the same size after the vector doubled, hiding an understated reservation, and the item-4 tests missed the PN-to-LID case entirely for the store reason above. A test that passes against the broken version proves nothing, and both were caught by actually running the mutation rather than assuming.Edge cases covered across the items: empty device list, one device, several devices; empty message id; unicode ids; a reporting token with no fields and one with a nested field; an address that fits inline, one that spills, and equality/hash between the two representations.
Verification
cargo fmt --all --check,cargo clippy --workspace --all-targetswith zero warnings, and green suites:whatsapp-rust1230,wacore1262,wacore-libsignal195,wacore-binary113.Review findings folded in after opening
Debugprinted the buffer, not the address. Resetting an address only rewinds the length, so the tail of an inline buffer still held the bytes of whoever it named before. The derivedDebugprinted the whole array, which means a log line formatting an address, or an error embedding one such asSessionNotFound, could show an unrelated peer's JID.ProtocolAddress::emptycollided as a map key.Hash,EqandOrdread the rendered string, soempty(1)andempty(2)compared alike. The device suffix is written up front now.FuturesUnorderedin completion order; those tests passed only because the current-thread runtime never let the futures pend. They now assert the half boundary, which is guaranteed, and membership within each half, plus a count so a set cannot hide a duplicate.capacity() == len()can fail on an allocator that rounds a request up. The property worth pinning, that the reservation matches the bytes written, is now adebug_assertinside the extractor that every calling test exercises.session_guards_fortrusted its callers. A duplicate key would await a lock the same call already holds: a send that never returns rather than a panic. Adebug_assertmakes it fail loudly.Deliberately left out
ProtocolAddressto 64 bytes (inline 39 + au16length). It would fit a cache line but trades inline coverage on a type that is mostly a map key. One constant if wanted.