perf(messages): draw the pad length from the thread RNG - #1189
Conversation
random_pad_len seeded a fresh StdRng to produce a single byte: 32 bytes of seed pulled from the thread generator, a full ChaCha12 key schedule and a 256-byte block generated, all so it could return (x & 0x0F) + 1. Use the thread-local generator directly. It is the same already-seeded ChaCha12 CSPRNG, so this is not a weakening of the entropy source, and the distribution is untouched. The identical fix already exists in request.rs for message_id_at; it was never applied here. Measured on a pinned core: 759 -> 41 retired instructions per call under callgrind, 99.1 -> 3.3 ns per call. The pad is drawn twice per DM send.
📝 WalkthroughWalkthrough
ChangesPadding randomness
Estimated code review effort: 1 (Trivial) | ~5 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 |
📦 Binary size report
.text per crate
Baseline: |
|
@codspeedbot explain why this not appears as faster |
Short answer: CodSpeed did measure it. Nothing was badged as an improvement because the effect is only a meaningful fraction of one benchmark in the suite, and even there it stayed under the threshold this repo flags at. Comparing this PR's run (
The flamegraphs make it unambiguous. On main, The instruction sub-component behaves exactly as your callgrind work predicted, and it is per-call additive: So why no badge:
The Memory instrument cannot show anything either way: seeding Net: the report reading "no change" is the suite telling you the win is 0.2% of a send, which is what the PR description already claims. Summary
Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot. |
|
Adding the local numbers while the bot digs, since they explain it without needing the flamegraphs. The change removes ~719 retired instructions per call under callgrind (759.5 -> 40.7, taken as the slope between a 1k and a 101k call run so process startup cancels). The pad is drawn twice per DM send, so ~1438 instructions per Against the timed region that is small. The other half is that base and head did not run on the same hardware. The compare for this run lists Overall run impact came out at -0.14%: right direction, too small to badge. That matches what the PR body claims. It is worth taking because it deletes pure work at zero risk, not because 190 ns per send is going to be visible anywhere. Generated by Claude Code |
Summary
MessageUtils::random_pad_lenbuilt a whole CSPRNG to produce a single byte.rand::make_rng::<StdRng>()pulls a 32 byte seed from the thread generator, runs a full ChaCha12 key schedule and generates a 256 byte block, and all of that existed so the next line could return(x & 0x0F) + 1.It now reads the thread-local generator directly. Same ChaCha12 CSPRNG, already seeded, so the entropy source is not weakened, and the distribution is untouched: still uniform over
1..=16.This exact fix already exists in the tree.
request.rs:219-222does it formessage_id_at, with the comment explaining why. It was applied there and never here.Changes
wacore/src/messages.rs:random_pad_lenusesrand::rng()instead of seeding a freshStdRngper plaintext. The comment justifying the(x & 0x0F) + 1distribution and the test that covers it are unchanged.Cost
Per call, isolated harness, 3 rounds of 2000 samples of 1000 calls each, core pinned with
taskset:make_rng::<StdRng>()rand::rng()Instruction counts are callgrind, taken as the slope between a 1k call and a 101k call run so process startup cancels out. Deterministic, and therefore the number worth trusting.
random_pad_lenruns twice per DM send:dm_plaintexts_from_encodedpads the recipient plaintext and the own-devices plaintext separately, sopad_message_v2is called once for each. That predicts about 1438 instructions off a DM send, and callgrind over the before/after bench binaries agrees: the whole-process slope forbench_dm_senddiffers by 1439 instructions per iteration.End to end, from CodSpeed's deterministic instrument (
3fa980dagainst baseline0247452):bench_dm_recv_steady(control, untouched)bench_encode_and_pad(one pad draw)bench_dm_send_encode_work[text_reply](two draws)bench_dm_sendThe saving is per pad draw and additive, and the control holding at +0.01% is what makes the rest readable. On main's flamegraph
bench_encode_and_padcarriesrand::make_rng::<StdRng>at 811 ns pluschacha20::ChaChaCore<R12>::generateat 627 ns, 8.8% of that benchmark; on the PR neither frame exists andrandom_pad_lenis a single 216 ns leaf.bench_encode_and_padreads 16.16 / 16.16 / 16.42 / 16.42 µs over the last four main runs, so 14.99 µs is outside baseline drift.Nothing is badged as an improvement because -8.7% on one micro-bench did not clear this repo's CodSpeed threshold, and the run-level aggregate averages it across ~210 benchmark/instrument pairs.
Locally the wall-clock benches could not resolve this and I am not claiming anything from them: 3 interleaved before/after rounds at
--min-time 3on a pinned core put the control's own spread at about 11% between rounds, well above the roughly 190 ns being looked for. That is a property of the box, not of the change; CodSpeed's instrument is what settles it.Magnitude, plainly: about 0.2% of a DM send. At any realistic message rate it is invisible. It is worth taking because it is a one line change that deletes pure work at zero risk, not because 190 ns matters.
Follow-ups (not touched here)
Other non-test
make_rng::<StdRng>()sites exist on or near send paths. Each has its own call frequency and needs its own measurement, so they are deliberately left alone:wacore/src/send/encrypt.rs:672wacore/src/send/group.rs:522,:692wacore/src/crypto.rs:80,:88wacore/src/store/signal_cache.rs:20Validation
cargo fmt --allcargo test -p wacore --lib: 1319 passed, 0 failed.random_pad_len_is_uniform_1_to_16passes with no adaptation, which is the point: this changes the entropy source, not the contract. No new test was added, since the property that matters (uniform over1..=16, 16 reachable) is already covered and a second test asserting the same thing would be redundant.cargo clippy --workspace --all-targetsdoes not complete in this environment: an unrelated crate's build script needsalsa.pcand the system package is absent. Full matrix left to CI, where Clippy Linter, Build & Lint (all features) and Feature Matrix are green.CI is green apart from Semver Checks (informational), which already fails on
mainat this PR's base commit02474521efromwaprotodrift against the last published release.