bench: deterministic rng and hashers; pin and shard the CodSpeed CI - #950
Conversation
Bench code (codspeed.io/docs -> reducing-variance / regression-causes): - libsignal_benchmark seeded every run from entropy (27 make_rng sites), so vartime signature paths measured different instruction counts per run; bench_rng() derives per-call-site StdRng streams from a fixed base seed. Run-to-run callgrind variance on bench_group_decrypt_message drops from ~6,000 to ~160 Ir (the residual is divan's timer calibration, absent under the instrumented runner). - Bench MemStores hashed with the default RandomState, whose per-process seed shuffles bucket layout (and cache behavior) between runs; a fixed DefaultHasher keeps the layout stable. Trait-mandated signatures keep the std type. CI (codspeed.yml): - runs-on pinned to ubuntu-24.04 (image drift shows up as toolchain-shaped regressions). - glibc malloc adaptive thresholds frozen via MALLOC_* env: mmap/trim/arena decisions vary with allocation history under both instruments. - Unit benches split into two package shards in the same workflow (same OIDC auth), roughly halving the serial bench wall time; CodSpeed merges shards into one run. - Docs-only changes no longer trigger the workflow.
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughLook, benchmarks that aren't reproducible aren't real benchmarks. This PR makes CodSpeed skip doc-only changes, freezes glibc malloc thresholds, and shards the benchmarks job into ChangesDeterministic Benchmarks and Sharded CodSpeed Workflow
Estimated code review effort: 2 (Simple) | ~15 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 |
|
| Filename | Overview |
|---|---|
| wacore/libsignal/benches/libsignal_benchmark.rs | All 27 rand::make_rng / rand::random call sites replaced with deterministic bench_rng(); DetHashMap applied to all five in-memory stores; registration_id now drawn from the same seeded rng. |
| .github/workflows/codspeed.yml | Runner pinned to ubuntu-24.04, MALLOC thresholds frozen, benchmarks split into two shards with fail-fast: false, cargo-codspeed bumped to 5.0.1, paths-ignore added for docs. |
| wacore/benches/send_receive_benchmark.rs | All five MemStore structs switched from HashMap (RandomState) to DetHashMap (BuildHasherDefault); RNG was already deterministic in this file. |
| Cargo.toml | codspeed-divan-compat version bumped from 4.7.0 to 5.0.1 to match the new cargo-codspeed CLI. |
| Cargo.lock | Lock file updated for codspeed* 5.0.1, colored 3.1.1 (drops lazy_static), and removal of windows-sys 0.59.0; all checksums updated. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CodSpeed trigger] --> B{paths-ignore check}
B -- docs only --> Z[Skip workflow]
B -- code changed --> C[Matrix 2 shards]
C --> D[core shard]
C --> E[proto-signal shard]
D --> F[build simulation + memory]
E --> G[build simulation + memory]
F --> H[codspeed run]
G --> I[codspeed run]
H --> J[CodSpeed merges shards]
I --> J
style J fill:#4caf50,color:#fff
style Z fill:#9e9e9e,color:#fff
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[CodSpeed trigger] --> B{paths-ignore check}
B -- docs only --> Z[Skip workflow]
B -- code changed --> C[Matrix 2 shards]
C --> D[core shard]
C --> E[proto-signal shard]
D --> F[build simulation + memory]
E --> G[build simulation + memory]
F --> H[codspeed run]
G --> I[codspeed run]
H --> J[CodSpeed merges shards]
I --> J
style J fill:#4caf50,color:#fff
style Z fill:#9e9e9e,color:#fff
Reviews (2): Last reviewed commit: "bench: random comes from RngExt in rand ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 4acef4bfe2
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/codspeed.yml:
- Around line 69-81: The CodSpeed workflow currently interpolates
matrix.shard.packages directly inside the shell command, which triggers
template-injection risk; update the Build the benchmark targets step and the
CodSpeedHQ/action@v4 run input to read the package list from an environment
variable instead of embedding the expression in the run string. Use the existing
workflow job context to pass matrix.shard.packages into env, then reference that
env variable in both cargo codspeed build and cargo codspeed run so the shell
never sees the raw template expansion.
In `@wacore/libsignal/benches/libsignal_benchmark.rs`:
- Around line 259-264: `User::new` still has a nondeterministic entropy source
because `registration_id` is generated with `rand::random::<u32>()` instead of
the local `bench_rng()`. Update the `registration_id` assignment in `User::new`
to draw from the same benchmark RNG used for `identity_key_pair`, so all
benchmark user state is fully deterministic and consistent.
🪄 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: ebdaf031-d81e-417a-9631-cfb65357376a
📒 Files selected for processing (3)
.github/workflows/codspeed.ymlwacore/benches/send_receive_benchmark.rswacore/libsignal/benches/libsignal_benchmark.rs
📦 Binary size report
.text per crate
Baseline: |
Merging this PR will improve performance by 27.59%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | bench_unpad_message_ref |
215.3 ns | 165.3 ns | +30.25% |
| ⚡ | Simulation | send_and_receive[1] |
1,169.1 µs | 935.4 µs | +24.98% |
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/bench-determinism (22a8bd5) with main (0ce4907)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Confidence score: 4/5
- In
wacore/libsignal/benches/libsignal_benchmark.rs,User::newstill usesrand::random::<u32>()forregistration_id, so benchmark inputs can vary run-to-run and make performance regressions harder to trust or compare; deriveregistration_idfrom the seeded benchrngbefore merging. - In
.github/workflows/codspeed.yml, relying on default matrixfail-fast: truecan cancel sibling shards after one failure, which hides full benchmark signal and slows diagnosis; setstrategy.fail-fast: falseso both shard results are always reported.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
AtomicU64 is a disallowed type in this repo (no native 64-bit atomics on Xtensa/ESP32); a u32 counter is plenty for call-site streams. Pin cargo-codspeed and codspeed-divan-compat to the latest release (5.0.1) instead of 4.7.0, keeping CLI and harness crates on the same major.
registration_id still drew from the ambient rng and is varint-encoded into prekey bundles, shifting payload sizes between runs. fail-fast: false keeps one shard's failure from cancelling the other's upload (CodSpeed merges shards into one run); the shard package list moves through an env var per actions hardening guidance.
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Auto-approved: Benchmark-only changes: deterministic RNG/hashers for reproducibility, CI config (pinned runner, malloc settings, sharding), and minor dev-dependency bumps.
Re-trigger cubic
1 participant
Applies the CodSpeed variance-reduction guidance (regression-causes / reducing-variance / sharded-benchmarks docs) to the bench suite and its CI, after the buffa-merge investigation showed 4 of the 6 reported regressions were measurement artifacts rather than code changes.
Bench code
Deterministic RNG in
libsignal_benchmark. All 27 setup sites usedrand::make_rng(entropy-seeded), so every run generated different key material. Signature verification is vartime over scalar bits, so the same benchmark legitimately executed a different instruction count on every run.bench_rng()now derives a distinctStdRngstream per call site from a fixed base seed (distinct streams so no two parties ever share keys; deterministic because divan's setup order is fixed).Measured (callgrind, same binary, two consecutive runs of
bench_group_decrypt_message):The residual is divan's wall-time calibration, which the CodSpeed instrumented runner does not execute.
Fixed-seed hashers for bench MemStores. The in-memory stores in
send_receive_benchmarkandlibsignal_benchmarkusedHashMapwith the defaultRandomState, whose per-process seed shuffles bucket layout, probe sequences, and therefore cache behavior between runs. They now useBuildHasherDefault<DefaultHasher>(SipHash with fixed keys, std-only). Trait-mandated signatures (fetch_prekeys) keep the std type.send_receive_benchmarkalready had a deterministicBenchRng; this brings the rest of the suite to the same standard.CI (codspeed.yml)
runs-onpinned toubuntu-24.04: image drift (glibc, system libs) reads as toolchain-shaped regressions. The Rust toolchain and cargo-codspeed were already pinned.MALLOC_ARENA_MAX/MMAP_THRESHOLD_/TRIM_THRESHOLD_/TOP_PAD_): mmap-vs-brk and trim decisions depend on allocation history and surface as spurious deltas under both instruments.core= wacore+noise,proto-signal= binary+libsignal+appstate), roughly halving the serial bench wall time (the single job was taking 30-60 min); CodSpeed merges shards into a single run per the sharded-benchmarks contract.paths-ignorefor docs-only changes.Not done (documented for later)
bench_unpad_message_refswinging -10% on a protobuf-only change) is now understood and cheap to triage instead.random_pad_len(1..=16 pad) still adds a few bytes of legitimate per-run variance to send-path benches; benchable-seams for it were not worth the intrusion.