bench(send): measure the shapes production actually sends - #1186
Conversation
Three fidelity gaps, each of which made a real cost invisible or overstated one that is not paid. **`account: None` charged every send an extra session checkout.** `prepare_peer_stanza` guards its pkmsg pre-flight with `account.is_none() && pkmsg_would_be_emitted(..)`, and that second call is a full `SessionCheckout::load` + `commit()`. A paired device always passes `Some` (`device_snapshot.account.as_deref()`), so the `&&` short-circuits and production never runs it. Measured: 3.007 -> 2.363 µs, **-21%** — the old baseline carried ~27% of work no send performs. Fixed here and in the group benches, which passed `None` for the same reason. **Nothing benchmarked the real 1:1 send.** The bench named `dm_send` called `prepare_peer_stanza`, which is the path to our OWN devices: no fan-out, no participants tree, no reporting token. It is now `bench_peer_send`, honestly named, and `bench_dm_send` / `bench_dm_send_4_devices` cover `prepare_dm_stanza`, which is what a message to a contact goes through. Worth noting what that exposed: a DM to 4 devices (14.1 µs) costs about the same as a group send to 10 (14.2 µs), because DM encrypts per device while a group sends one skmsg. **`bench_dm_recv` measures a DH ratchet step, not a received message.** Its comment claimed steady state, but Bob has never decrypted on that sending chain, so every iteration pays two X25519 agreements and a key generation. `bench_dm_recv_steady` burns that step in setup and measures the symmetric-only decrypt an active conversation actually pays: **77.7 µs vs 1.87 µs, a 42x difference**. Any improvement to the common receive path was previously diluted into invisibility. Both shapes are real, so both are kept, and the old one's comment now says which is which. CodSpeed baselines for `bench_dm_send`, `bench_peer_send` and the group sends will shift; that is the point.
📦 Binary size report
.text per crate
Baseline: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ 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
WalkthroughDM and group benchmarks now pass deterministic paired-device identities into stanza preparation. The benchmark also adds a steady-state DM receive path that advances the ratchet before measuring symmetric decryption. ChangesSend and receive benchmark coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 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 |
![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: 62dad04add
ℹ️ 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".
Merging this PR will degrade performance by 40.05%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| 🆕 | Memory | bench_dm_recv_steady |
N/A | 2 KB | N/A |
| 🆕 | Memory | bench_peer_send |
N/A | 2.6 KB | N/A |
| 🆕 | Memory | bench_dm_send_5_way_fanout |
N/A | 10.2 KB | N/A |
| 🆕 | Simulation | bench_dm_recv_steady |
N/A | 67 µs | N/A |
| 🆕 | Simulation | bench_peer_send |
N/A | 83 µs | N/A |
| 🆕 | Simulation | bench_dm_send_5_way_fanout |
N/A | 278.7 µs | N/A |
| 👁 | Memory | bench_dm_send |
2.6 KB | 4.3 KB | -40.12% |
| 👁 | Simulation | bench_dm_send |
86.4 µs | 144 µs | -39.98% |
Comparing bench/production-shaped-send-receive (7d70c12) with main (479fcc7)
Footnotes
-
2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wacore/benches/send_receive_benchmark.rs (1)
1037-1046: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
bench_account()is being built inside the measured closure — move it intoGrpSendData.
run_group_sendis whatbench_refscalls every iteration, so Line 1037 allocates four freshVecs per sample and charges them to the group-send baseline. That is exactly the kind of non-production work this PR set out to stop paying for, andsetup_group_recvalready does it right by building the account in setup. Every other fixture here (DmSendData.account,DmFanoutData.account) holds it as a field — do the same forGrpSendDataso the number we ship to CodSpeed is the stanza prep and nothing else.⚡ Hold the account in the fixture instead
Add the field to
GrpSendDataand populate it in the group-send setup (alongside the existingresolver/runtimefields):struct GrpSendData { // ...existing fields... account: wa::ADVSignedDeviceIdentity, } // in the setup that builds GrpSendData: account: bench_account(),Then in
run_group_send:- let account = bench_account(); let result = futures::executor::block_on(prepare_group_stanza( &runtime, &mut stores, &d.resolver, GroupStanzaRequest { group: &group_info, own_jid: &own_jid, own_lid: &own_jid, - account: Some(&account), + account: Some(&d.account),Note the borrow order: take
d.accountbeforestoresmutably borrowsd.alice, or bindlet account = &d.account;above theSignalStoresconstruction.🤖 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/benches/send_receive_benchmark.rs` around lines 1037 - 1046, Move account construction out of the measured run_group_send path by adding an account field to GrpSendData and initializing it with bench_account() in the group-send setup alongside the existing fixture fields. Update run_group_send to borrow d.account, binding it before constructing SignalStores if needed to avoid conflicting borrows, and pass that reference to GroupStanzaRequest instead of creating a fresh account per iteration.
🤖 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/benches/send_receive_benchmark.rs`:
- Around line 967-985: Extract the duplicated encryption-and-ciphertext mapping
logic from setup_dm_recv and the local encrypt_one in setup_dm_recv_steady into
one file-level encrypt_one helper. Update both receive setup paths to call that
shared helper, preserving the existing padding, message encryption, wire
serialization, and "msg"/"pkmsg" type mapping behavior.
---
Outside diff comments:
In `@wacore/benches/send_receive_benchmark.rs`:
- Around line 1037-1046: Move account construction out of the measured
run_group_send path by adding an account field to GrpSendData and initializing
it with bench_account() in the group-send setup alongside the existing fixture
fields. Update run_group_send to borrow d.account, binding it before
constructing SignalStores if needed to avoid conflicting borrows, and pass that
reference to GroupStanzaRequest instead of creating a fresh account per
iteration.
🪄 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: e64852cb-34dd-485b-9476-474fe3861e06
📒 Files selected for processing (1)
wacore/benches/send_receive_benchmark.rs
Five review findings, all of which made the new benches measure something other than what they claim. **The DM fixtures used unacknowledged sessions.** `establish_session` alone leaves `pending_pre_key` set, so every outbound stayed a `pkmsg` — the new "repeat send" benches were measuring first-message prekey wrapping and `<device-identity>` serialisation forever. That is the exact class of error this PR exists to fix, in the code fixing it. They now use `establish_bidirectional`, and the fixture asserts `!pkmsg_would_be_emitted` per device so a regression cannot quietly restore it. **`bench_account()` ran inside the measured group closure**, allocating four `Vec`s per iteration that production does not: the real path borrows a cached account. Moved into `GrpSendData`, as the DM fixtures already did. **The name overstated the fan-out.** `setup_dm_fanout(4)` builds four recipient devices *plus* one own companion, so it is five pairwise encryptions. Renamed to `bench_dm_send_5_way_fanout`. **The no-account rationale was wrong.** It claimed the DM and group paths refuse a pkmsg outright. They do not: `prepare_dm_stanza` and the `BestEffort` group policy drop the `<device-identity>` child and send anyway; only the peer path and `Required` distribution propagate. Stating it wrongly in a fixture comment is how the next fixture change goes wrong. **`encrypt_one` was duplicated** between the two receive setups, so the ratchet and steady-state benches could drift apart on padding or enc-type mapping. One file-level helper now.
1 participant
Three fidelity gaps in
send_receive_benchmark, found while profiling the Signal paths. Each one either hid a real cost or charged one production does not pay, so the baselines they produce are not the thing we think we are tracking.No production code changes — benches only.
1.
account: Nonecharged every send an extra session checkoutprepare_peer_stanzaguards its pre-flight with:That second call is a full
SessionCheckout::load+commit(). A paired device always passesSome(src/send/mod.rs:device_snapshot.account.as_deref()), so the&&short-circuits and production never runs it. The bench passedNone.account: None(before)account: Some(production)−21% — the old baseline carried ~27% of work no send performs. Fixed here and in the group benches, which passed
Nonefor the same reason.2. Nothing benchmarked the real 1:1 send
The bench called
dm_sendinvokedprepare_peer_stanza— the path to our own devices. No fan-out, no participants tree, no reporting token. The actual DM path (prepare_dm_stanza) had no coverage at all.bench_peer_send— the old bench, honestly named.bench_dm_send/bench_dm_send_5_way_fanout— the contact path, with the recipient's devices and one own companion.Worth noting what that immediately exposed: a 5-way DM fan-out (12.35 µs) costs about the same as a group send to 10 (12.70 µs) — DM encrypts per device, a group sends one skmsg. That relationship was not visible before.
3.
bench_dm_recvmeasures a DH ratchet step, not a received messageIts comment claimed "realistic steady-state", but Bob has never decrypted on that sending chain, so every iteration pays two X25519 agreements plus a key generation.
bench_dm_recv(ratchet step)bench_dm_recv_steady(symmetric only)A 43× difference. Any improvement to the decrypt path an active conversation actually uses was diluted into invisibility. Both shapes are real — the ratchet one is the first message after the peer replies — so both are kept, and the old one's comment now says which it is instead of claiming to be the other.
Result
Reported as
fastestover threetaskset -c 4rounds, stable to ±1%. Anotherbuild was running on the box during measurement, which inflates medians without
touching the floor;
bench_group_send_256andbench_group_recvwere the twothat still moved between rounds and their numbers are given as ranges. The
before/after figures in sections 1 and 3 were taken on a quiet box.
Clippy clean on
--benches,cargo test -p wacore --lib1297 passed.CodSpeed baselines will move for
bench_dm_send,bench_peer_sendand the group sends. That is the intent: the previous numbers measured a shape we do not send.Review pass
Five findings, all valid, all fixed — one of them serious enough to name:
The new DM fixtures used unacknowledged sessions.
establish_sessionalone leavespending_pre_keyset, so every outbound stayed apkmsg: the "repeat send" benches were measuring first-message prekey wrapping and<device-identity>serialisation. That is precisely the class of error this PR exists to fix, committed inside the fix. They now useestablish_bidirectional, and the fixture asserts!pkmsg_would_be_emittedper device so it cannot silently regress.Also:
bench_account()was being called inside the measured group closure (fourVecs per iteration that production does not allocate — it borrows a cached account);bench_dm_send_4_devicesactually performed five pairwise encryptions and is renamed accordingly; the no-account rationale claimed the DM and group paths refuse a pkmsg outright when in factprepare_dm_stanzaandBestEffortdrop the<device-identity>child and send anyway; andencrypt_onewas duplicated across the two receive fixtures, which would let the ratchet and steady-state benches drift apart.