Skip to content

Add CodSpeed performance measurement setup - #828

Merged
jlucaso1 merged 4 commits into
mainfrom
codspeed/wizard-1781094525932
Jun 10, 2026
Merged

Add CodSpeed performance measurement setup#828
jlucaso1 merged 4 commits into
mainfrom
codspeed/wizard-1781094525932

Conversation

@codspeed-hq

@codspeed-hq codspeed-hq Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR sets up CodSpeed for continuous performance measurement on every pull request and on pushes to main.

The repository already had a substantial benchmark suite, but it was built on iai-callgrind, which CodSpeed does not support. The core of this change is migrating those benchmarks to the codspeed-divan-compat harness so the existing coverage can be measured by CodSpeed, with per-benchmark flamegraphs and low-variance results.

Changes

  • Benchmark harness migration: Replaced iai-callgrind with codspeed-divan-compat (exposed as divan) in the workspace dependencies, and converted all five benchmark suites to the divan API:

    • wacore/binary/benches/numeric_attr_benchmark.rs
    • wacore/binary/benches/binary_benchmark.rs
    • wacore/benches/reporting_token_benchmark.rs
    • wacore/benches/send_receive_benchmark.rs
    • wacore/libsignal/benches/libsignal_benchmark.rs

    The benchmarked workloads themselves are unchanged. Parameterized cases that previously used #[bench::name(...)] are expressed as divan benches with with_inputs(...) so setup cost stays out of the measurement.

  • CI workflow: Added .github/workflows/codspeed.yml, which builds the benchmark targets with cargo codspeed build and runs them through CodSpeedHQ/action@v4 in simulation mode using OIDC authentication. It runs on ubuntu-latest, which is appropriate for simulation (instruction-count) mode.

  • Removed the obsolete pipeline: The previous benchmarking flow relied on iai-callgrind output post-processed by custom scripts and github-action-benchmark. Since the harness no longer produces iai-callgrind output, that pipeline would fail permanently. Removed:

    • .github/workflows/benchmark.yml
    • .github/workflows/benchmark-comment.yml
    • .github/scripts/iai-to-benchmark-json.py
    • .github/scripts/bench-comment.py

    Note: .github/workflows/bench-integration.yml is a separate end-to-end harness and was intentionally left untouched.

  • README badge: Added a CodSpeed badge linking to the project dashboard.

Validation

All five benchmark suites build and run locally under CodSpeed in simulation mode:

cargo codspeed build -p wacore -p wacore-binary -p wacore-libsignal
codspeed run --mode simulation -- cargo codspeed run

This produces results for every benchmark with no errors.

Next steps

  • Once this PR is merged, CodSpeed will report performance changes directly on each pull request.
  • Consider extending coverage to any additional hot paths that are not yet benchmarked.

@codspeed-hq

codspeed-hq Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Congrats! CodSpeed is installed 🎉

🆕 142 new benchmarks were detected.

You will start to see performance impacts in the reports once the benchmarks are run from your default branch.

Detected benchmarks


ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.


Open in CodSpeed

@codspeed-hq
codspeed-hq Bot marked this pull request as ready for review June 10, 2026 12:56
@jlucaso1

Copy link
Copy Markdown
Collaborator

@claude review

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Claude finished @jlucaso1's task in 3m 46s —— View job


Claude PR Review

  • Read changed files (workflow, Cargo.toml, benchmarks)
  • Verify benchmark workload equivalence (iai-callgrind → divan)
  • Check CI workflow correctness
  • Verify removed files don't leave dangling references
  • Post review feedback

Overall: clean, well-scoped harness migration. The benchmark workloads are preserved verbatim — only the wrapper attributes change. CodSpeed has already accepted the run (67 benchmarks detected), which is a good signal. A few leftover references to the old harness slipped through; the workflow looks correct.

Leftover iai-callgrind references

These were missed by the cleanup:

  1. .github/workflows/copilot-setup-steps.yml:46-49 — still installs valgrind and iai-callgrind-runner@0.16.1. With the harness gone, this just slows down the Copilot setup with unused tools. Since the PR was already touching workflows, this one should go too. (I can't push edits to .github/workflows/* from this app, so this needs a manual change — drop the valgrind,iai-callgrind-runner@0.16.1 install step entirely.)

  2. Cargo.toml:218-223 — the [profile.bench] block is still documented as a workaround for iai-callgrind:

    # Benchmark profile: optimized but without aggressive LTO that breaks iai-callgrind
    [profile.bench]
    inherits = "release"
    lto = "thin"
    ...

    With iai-callgrind gone, the lto = "thin" justification disappears. cargo codspeed build uses its own profile so it isn't affected either way, but plain cargo bench will use this. Either delete the block (let it inherit defaults) or update the comment to reflect why thin is still wanted. Stale comment is the bigger issue. Fix this →

  3. wacore/benches/send_receive_benchmark.rs:551-553 — the comment still mentions iai-callgrind:

    // Built once in setup so the measured body excludes thread-pool startup
    // (iai-callgrind would otherwise charge the syscalls to the encrypt path).
    runtime: BenchRuntime,

    The intent (build the runtime in setup, exclude it from the measured body) is still correct under divan's with_inputs / bench_values model, but the parenthetical reference is now wrong. Fix this →

Setup-vs-body placement (carry-over from iai-callgrind, worth a second look)

Several benches in wacore/binary/benches/binary_benchmark.rs build their input inside the timed body rather than via with_inputs:

#[divan::bench]
fn bench_marshal_allocating() -> Vec<u8> {
    let node = create_large_node();              // <-- in the measured body
    black_box(marshal(black_box(&node)).unwrap())
}

This is identical to how iai-callgrind ran them in main, so it's not a regression — but under divan/CodSpeed the reported instruction count for bench_marshal_allocating now includes the cost of building the 20-child large-node tree (and similarly for *_long_string, *_huge_bytes_allocating, *_many_children_allocating, bench_unpack_*, bench_get_children_by_tag). If the intent is to measure only marshal() (etc.), these should follow the same bencher.with_inputs(create_large_node).bench_values(|node| marshal(&node)) pattern that bench_unmarshal_small/bench_roundtrip_* already use. If the intent is to measure construct+marshal as a unit, no change needed — but a one-line comment per group stating that would prevent future "why is bench_marshal_allocating slower than bench_marshal_long_string even though the strings are bigger?" confusion. Same applies to the bench_marshal_huge_bytes_allocating family where vec![0x5A; 256 * 1024] is constructed inside each sample.

CI workflow (.github/workflows/codspeed.yml)

Looks good. Notes:

  • concurrency.group: codspeed-${{ github.head_ref || github.run_id }} correctly avoids canceling main-baseline runs (on push, head_ref is empty → falls back to per-run run_id, so cancel-in-progress is a no-op there). Good.
  • OIDC + simulation mode + ubuntu-latest is the right combination.
  • cargo codspeed build -p wacore -p wacore-binary -p wacore-libsignal is explicit and matches where the benches live. If new bench targets are added to other workspace crates (e.g. wacore-appstate, wacore-noise), this list will need updating. Consider whether --workspace --benches would be more future-proof — minor.
  • cache-targets: "false" means a clean rebuild every run; for CodSpeed measurement stability this is conservative-but-fine.
  • id-token: write is needed for OIDC and contents: read is sufficient; permissions are minimal. Good.

Other observations

  • bench-integration.yml is intentionally untouched and still uses benchmark-action/github-action-benchmark@v1 — confirmed it's a separate harness, fine.
  • [profile.profiling] (Cargo.toml:226) is unrelated, no change needed.
  • Cargo.lock churn (+211/-67) is consistent with pulling in codspeed-divan-compat's tree (clap/colored/etc.) — nothing odd there.
  • No iai_callgrind imports remain in benchmark sources. Removed scripts (iai-to-benchmark-json.py, bench-comment.py) have no other references in the repo.

Summary

Migration itself is solid. Before merging, I'd recommend:

  1. Remove valgrind,iai-callgrind-runner from copilot-setup-steps.yml.
  2. Update or remove the stale iai-callgrind reference in Cargo.toml's [profile.bench] comment.
  3. Update the stale iai-callgrind reference in send_receive_benchmark.rs:552.
  4. Optional: move per-sample input construction in the bench_marshal_* family into with_inputs so the reported numbers measure only the marshal path — or note explicitly that they measure construct+marshal.
    · branch: codspeed/wizard-1781094525932

@jlucaso1

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6d01cccb-ca2b-4e0a-880e-1e9f47290fab

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Benchmark infrastructure is being migrated from iai-callgrind to divan with CodSpeed integration. Old Python scripts and workflows removed. CodSpeed workflow added. Dependencies updated across workspace. All benchmark harnesses migrated to divan with #[divan::bench] attributes and bencher input patterns.

Changes

Benchmark Framework Migration from iai-callgrind to divan with CodSpeed

Layer / File(s) Summary
CodSpeed Workflow Setup & Old Infrastructure Removal
.github/workflows/codspeed.yml, README.md, .github/scripts/bench-comment.py
Old benchmark comment generator script removed. New CodSpeed GitHub Actions workflow added with nightly Rust, protoc, cargo-codspeed tooling, benchmark package builds, and CodSpeed simulation execution. CodSpeed badge added to README.
Workspace Dependency Updates
Cargo.toml, wacore/Cargo.toml, wacore/binary/Cargo.toml, wacore/libsignal/Cargo.toml
All workspace and crate-level dev-dependencies updated: iai-callgrind replaced with divan across four manifest files.
Reporting Token Benchmarks Migration
wacore/benches/reporting_token_benchmark.rs
Imports updated to divan::black_box and divan::main(). Benchmarks rewritten with #[divan::bench] using bencher.with_inputs() and bench_values() for content extraction, token generation, and message encoding variants.
Send/Receive Benchmarks Migration
wacore/benches/send_receive_benchmark.rs
Explicit fn main() delegates to divan::main(). DM send/receive and group send/receive benchmarks converted to #[divan::bench] with bencher input setup and bench_values closures. Group send expanded to explicit size variants (10/50/256) and SKDM distribution variants.
Binary Node Benchmarks Migration
wacore/binary/benches/binary_benchmark.rs
Imports updated to divan::black_box and main entry calls divan::main(). All marshal/unmarshal, attribute parser, round-trip re-marshal, child iteration, and JID optimization benchmarks converted to #[divan::bench] with with_inputs() and bench_values() pattern. Old library_benchmark_group! and Callgrind harness removed.
Numeric Attribute Benchmarks Migration
wacore/binary/benches/numeric_attr_benchmark.rs
Main entry point and imports updated for divan::main() and divan::black_box. Integer conversion benchmarks (u32/u64/i64) and proposed loop variants converted from #[library_benchmark] to #[divan::bench]. Explicit group registration and Callgrind configuration removed.
Signal Protocol Benchmarks Migration
wacore/libsignal/benches/libsignal_benchmark.rs
Main entry point delegates to divan::main(). All benchmarks converted from iai_callgrind macros to #[divan::bench] using bencher.with_inputs().bench_values(). Covers DM session establishment, encryption/decryption, group operations, full conversation flow, signatures, previous-session iteration, out-of-order decryption, PreKey processing, and message-key eviction. Old library_benchmark_group! and Callgrind main!() removed.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The migration is systematic and repetitive across benchmark files—same pattern applied consistently—but requires verification that benchmark logic is preserved across the harness swap and that the CodSpeed integration works correctly. New workflow setup needs validation against CodSpeed requirements. No public API changes; this is infrastructure and tooling-only.

Possibly related PRs

  • oxidezap/whatsapp-rust#189: Directly tied—removes .github/workflows/benchmark.yml and rewrites multiple wacore/benches/* files from iai_callgrind to divan.
  • oxidezap/whatsapp-rust#501: Updates wacore/benches/send_receive_benchmark.rs DM/group send/receive benchmarks, directly touching the same entrypoints migrated in this PR.
  • oxidezap/whatsapp-rust#584: Introduced numeric NodeValue conversion behavior benchmarked in wacore/binary/benches/numeric_attr_benchmark.rs, which this PR migrates to divan.

Suggested labels

performance


Look, here's the thing—this has to work flawlessly. We're ripping out the entire benchmark infrastructure and replacing it with something new. That's not something you do casually.

The CodSpeed workflow setup is clean and focused. The dependency migrations are straightforward. But every single benchmark file needs to be checked: the logic has to be identical, the measurements have to be valid, and we cannot have benchmarks that silently break or start reporting garbage numbers. That would be worse than having no benchmarks at all.

The migration pattern is consistent across all files, which is good—it means once we verify one, we understand the rest. But verify we must. Black box operations need to be in exactly the right places. Input setup functions need to produce the same data. And we need to trust that divan actually measures what iai-callgrind was measuring, or at least that we understand the differences.

CodSpeed gives us continuous tracking, which is solid—that's the real win here. But only if the benchmarks are correct.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly summarizes the main change: adding CodSpeed for performance measurement.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, detailing the benchmark harness migration, workflow changes, and validation results.
Docstring Coverage ✅ Passed Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codspeed/wizard-1781094525932

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Cargo.toml (1)

218-224: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Update the outdated comment.

This comment still mentions "iai-callgrind" but you're removing that entire framework in this PR. The profile configuration might still be relevant for divan, but the comment is misleading now. Update it to reflect the actual benchmark framework you're using, or remove the iai-callgrind reference.

📝 Suggested fix
-# Benchmark profile: optimized but without aggressive LTO that breaks iai-callgrind
+# Benchmark profile: optimized with thin LTO and debug symbols for benchmark measurements
 [profile.bench]
 inherits = "release"
 lto = "thin"
🤖 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 `@Cargo.toml` around lines 218 - 224, The comment above the [profile.bench]
section is outdated because it references "iai-callgrind"; update the comment to
reflect the current benchmark framework or remove that reference entirely.
Locate the comment near the [profile.bench] profile (the block configuring lto =
"thin", debug = 1, strip = false) and replace the sentence mentioning
"iai-callgrind" with a short description that matches the benchmarks you now run
(or delete the specific framework name), ensuring the comment accurately
documents why this profile exists and any tradeoffs (e.g., optimized bench
profile with thin LTO and debug symbols).
🤖 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:
- Line 29: The checkout step using actions/checkout@v6 currently leaves Git
credentials persisted; update that step to include the persist-credentials:
false input so credentials are not stored in the runner for subsequent steps.
Locate the step with uses: actions/checkout@v6 and add the persist-credentials:
false option (as an input under that step) to disable credential persistence
when checking out the repo.
- Around line 29-53: The workflow uses mutable action refs under OIDC (id-token:
write); replace each mutable ref—actions/checkout@v6,
dtolnay/rust-toolchain@master, taiki-e/install-action@v2,
Swatinem/rust-cache@v2, and CodSpeedHQ/action@v4—with their corresponding
immutable full commit SHAs (keep the original tag/branch as a comment for
readability), ensuring the semantics and inputs (e.g., tool versions, toolchain)
remain unchanged; verify the SHA pins by fetching the commit for each repository
and update the workflow to use those SHAs so the OIDC-enabled workflow only
references immutable action commits.

---

Outside diff comments:
In `@Cargo.toml`:
- Around line 218-224: The comment above the [profile.bench] section is outdated
because it references "iai-callgrind"; update the comment to reflect the current
benchmark framework or remove that reference entirely. Locate the comment near
the [profile.bench] profile (the block configuring lto = "thin", debug = 1,
strip = false) and replace the sentence mentioning "iai-callgrind" with a short
description that matches the benchmarks you now run (or delete the specific
framework name), ensuring the comment accurately documents why this profile
exists and any tradeoffs (e.g., optimized bench profile with thin LTO and debug
symbols).
🪄 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

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: faaaf5d6-3526-4204-9a89-6041e357964c

📥 Commits

Reviewing files that changed from the base of the PR and between 50e0db4 and 881ee51.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (15)
  • .github/scripts/bench-comment.py
  • .github/scripts/iai-to-benchmark-json.py
  • .github/workflows/benchmark-comment.yml
  • .github/workflows/benchmark.yml
  • .github/workflows/codspeed.yml
  • Cargo.toml
  • README.md
  • wacore/Cargo.toml
  • wacore/benches/reporting_token_benchmark.rs
  • wacore/benches/send_receive_benchmark.rs
  • wacore/binary/Cargo.toml
  • wacore/binary/benches/binary_benchmark.rs
  • wacore/binary/benches/numeric_attr_benchmark.rs
  • wacore/libsignal/Cargo.toml
  • wacore/libsignal/benches/libsignal_benchmark.rs
💤 Files with no reviewable changes (4)
  • .github/workflows/benchmark-comment.yml
  • .github/scripts/iai-to-benchmark-json.py
  • .github/scripts/bench-comment.py
  • .github/workflows/benchmark.yml

Comment thread .github/workflows/codspeed.yml
Comment thread .github/workflows/codspeed.yml
…e/jid/history-sync

Review fixes: the stale iai-callgrind references are gone (copilot setup
no longer installs valgrind, the bench profile comment states the real
rationale, the send_receive comment is harness-agnostic).

Wrong measurements fixed: every marshal bench built its node inside the
timed body (the 2048-child shape measured mostly construction), and the
compressed unpack bench deflated its own input per sample; all inputs
now come from with_inputs and the unpack fixtures are realistic frame
sizes. The 12-bench marshal matrix is trimmed to the production
strategy (marshal_auto) per payload shape plus one strategy-comparison
trio. The numeric-attr baseline-vs-proposed A/B file was a fossilized
experiment (the itoa path already shipped) and is deleted.

New coverage for hot paths that had none: app-state patch processing
with MAC validation and the LTHash batch math (new wacore-appstate
bench target), transport frame AES-GCM at stanza and media-chunk sizes
(new wacore-noise target), Jid parse/format/push_ad_to, the realistic
20k-message history-sync ingest (migrated from the stale local iai
file), the 1600-device participant hash, and message pad/unpad.

CI gains a second CodSpeed job running the memory instrument
(mode: memory + an instrumented build via --measurement-mode memory),
and both jobs build the two new bench packages.
@jlucaso1

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@jlucaso1

Copy link
Copy Markdown
Collaborator

@claude review

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Claude finished @jlucaso1's task in 3m 22s —— View job


Claude PR Review

  • Pull latest commits and inspect diff vs origin/main
  • Review new benchmark files (appstate, noise, history_sync, message_utils, jid)
  • Review changes to existing benchmarks since the previous review pass
  • Re-check leftover issues from the previous round
  • Check CodSpeed workflow correctness (incl. memory job)
  • Verify CI is green (7/7 passing)

The second commit (1354112e) cleanly addresses every actionable item from my previous review pass and substantially improves measurement correctness. This is a solid follow-up. CI is green on all 7 jobs, including both the simulation and memory CodSpeed runs.

Previous-round leftovers — all resolved ✅

  • Cargo.toml:218 — comment rewritten to "release-grade codegen, but thin LTO keeps bench builds fast" — accurate, no more iai-callgrind reference.
  • .github/workflows/copilot-setup-steps.ymlvalgrind + iai-callgrind-runner install step removed.
  • wacore/benches/send_receive_benchmark.rs:551-554 — comment is now harness-agnostic ("building the pool inside the bench body would charge its syscalls to the encrypt path").
  • wacore/binary/benches/binary_benchmark.rs — the per-sample input-construction issue I flagged was fully fixed. Every marshal_* bench now precomputes its node in with_inputs. The 12-bench marshal_* matrix was trimmed to marshal_auto per payload shape plus a single strategy-comparison trio on the large shape — a much cleaner, less noisy surface. bench_unpack_compressed now pre-builds its compressed fixture in setup (was deflating per-sample before).

New coverage — well-targeted

The new bench files line up cleanly with the actual hot paths:

  • wacore-appstate/benches/appstate_benchmark.rs — LTHash batch math at upload-scale (812 MACs), HKDF expansion, full inbound patch processing with MAC validation on a realistic 50-mutation patch. APIs all check out (WAPATCH_INTEGRITY.subtract_then_add, expand_app_state_keys, process_patch with closures returning Arc<ExpandedAppStateKeys>). Minor: the "812 MACs = one prekey-upload-sized batch" claim doesn't tie to an obvious constant — MAX_PRE_KEY_UPLOAD_BATCH = u16::MAX, MIN_PRE_KEY_COUNT is much smaller, so "prekey-upload-sized" is approximate. Not a blocker; the order-of-magnitude framing is the point.
  • wacore-noise/benches/noise_benchmark.rs — Transport-frame AES-256-GCM in-place encrypt/decrypt at 1.5 KB (stanza) and 64 KB (media chunk). API matches NoiseCipher::encrypt_in_place_with_counter / decrypt_in_place_with_counter. Clean.
  • wacore-binary/benches/jid_benchmark.rs — JID parse (4 shapes via args = [...]), to_string, to_non_ad_string, push_ad_to (the per-recipient fan-out formatter). All real APIs verified.
  • wacore/benches/history_sync_benchmark.rs — Realistic 500 × 40 history-sync ingest using wacore::history_sync::process_history_sync. sample_count = 5 is appropriate for a multi-MB protobuf+inflate pass. Good shape variation (1-byte vs 2-byte varint lengths) to exercise the scan path.
  • wacore/benches/message_utils_benchmark.rsparticipant_list_hash on 1600 devices, encode_and_pad, unpad_message_ref. APIs all match.

CodSpeed workflow

  • Adding the memory instrument job as a sibling is the right way to do this (instrumented build is incompatible with the simulation build, so separate jobs is correct).
  • cargo codspeed build -p ... -p wacore-appstate -p wacore-noise correctly extends to the new bench packages.
  • Both jobs use the same set of pinned-version tools (protoc@3.25.3, cargo-codspeed@4.7.0, nightly-2026-04-05) — reproducible.
  • The concurrency group still does the right thing (head_ref || run_id falls back per-run on push, so cancel-in-progress doesn't kill main-baseline runs).

Carry-over items from CodeRabbit's review

CodeRabbit flagged these on .github/workflows/codspeed.yml and I think both are worth applying, even though neither is essential to land:

  1. persist-credentials: false on actions/checkout@v6 (line 29 + line 62). Minor hardening — checkout's default leaves ${{ github.token }} in .git/config for subsequent steps. This workflow doesn't need it (it only builds & runs benches), and your other workflows that do push (none here) can opt in explicitly.
  2. Pin actions to full commit SHAs instead of tags/branches (actions/checkout@v6, dtolnay/rust-toolchain@master, taiki-e/install-action@v2, Swatinem/rust-cache@v2, CodSpeedHQ/action@v4). With id-token: write (OIDC), a maintainer compromise on any of those is more dangerous because the workflow can mint trust tokens. Standard practice is uses: actions/checkout@<sha> # v6.

Both are recommendations rather than blockers. I can't push edits to .github/workflows/* from this app — if you want them applied, either accept CodeRabbit's autofix or have a maintainer push.

Other observations

  • Cargo.lock and dep additions (codspeed-divan-compat pulling clap/colored/etc., wacore/noise test-util features) are all expected.
  • Test/bench-only fictitious data: phone numbers in the new benches use 5511… / 100000000000… shapes, no real PII.
  • BenchRuntime::sleep/spawn_blocking are unimplemented!() — fine, the bench paths explicitly don't hit them and a hit would loudly panic rather than silently fake a result.

Summary

LGTM to merge. The follow-up commit is comprehensive: every previous-review item resolved, measurement correctness improved (marshal/unpack inputs out of the timed body, strategy matrix trimmed), and the new coverage hits the right hot paths (appstate LTHash, noise transport, history-sync, JID, participant hash, pad/unpad) plus a separate memory-instrument CI job. Optional hardening: persist-credentials: false and SHA-pinned action refs on codspeed.yml.

--- · branch: codspeed/wizard-1781094525932

@codspeed-hq

codspeed-hq Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the note — I've verified the current state of .github/workflows/codspeed.yml on this branch.

Both checkout steps (in the benchmarks and benchmarks-memory jobs) already set persist-credentials: false:

- uses: actions/checkout@v6
  with:
    persist-credentials: false

This was applied in the latest commit (ci: do not persist checkout credentials in the codspeed jobs), so no further changes are needed. 👍

Summary
  • Warming up the performance wizard...
  • Verify persist-credentials in CodSpeed workflow — Confirmed both checkout steps in the CodSpeed workflow already set persist-credentials: false. No changes needed.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

@codspeed-hq

codspeed-hq Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Agreed — keeping CodSpeedHQ/action@v4 (and the other steps) tag-pinned is the right call here. It matches the convention used by every other workflow in this repo, and SHA-pinning a single file without a repo-wide policy plus a dependabot config to keep the pins fresh would just create inconsistency and stale references.

No changes needed on this point. The workflow already references the CodSpeed action by its latest major tag, which is exactly how we recommend pinning it absent a repo-wide SHA-pinning policy. Happy to revisit if you ever adopt SHA-pinning across all workflows in a dedicated PR.

Summary
  • Warming up the performance wizard...
  • Reviewed reviewer note on action pinning — The comment documents a deliberate decision to keep GitHub Action references tag-pinned for repo-wide consistency. No code change is requested; the existing setup already follows repo conventions.
  • Handling comment — Comment handling finished successfully

Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant