ci: diff CodSpeed base vs. head on the same runner for PRs by moreal · Pull Request #8658 · RustPython/RustPython · GitHub
Skip to content

ci: diff CodSpeed base vs. head on the same runner for PRs - #8658

Draft
moreal wants to merge 13 commits into
RustPython:mainfrom
moreal:codspeed-local-base-head-diff
Draft

ci: diff CodSpeed base vs. head on the same runner for PRs#8658
moreal wants to merge 13 commits into
RustPython:mainfrom
moreal:codspeed-local-base-head-diff

Conversation

@moreal

@moreal moreal commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Summary

CodSpeed's own comparison is cross-run: base and head can each draw a different GitHub Actions machine, and glibc's IFUNC-selected memcpy/malloc implementations make even Valgrind/Callgrind instruction counts move with which CPU was drawn (see codspeed.io/blog/why-glibc-faster-github-actions). That can surface as a reported regression that is really just base and head having landed on different hardware.

  • Adds a local-diff job to .github/workflows/codspeed.yaml that, on pull requests, measures base and head back-to-back in the same job (same runner, same glibc, same kernel) using a stock Valgrind wrapped directly around the built [[bench]] targets (scripts/codspeed-local-diff.py), independent of CodSpeed's own runner/parser.
  • Adds .github/workflows/codspeed-comment.yaml, a workflow_run-triggered workflow that posts/updates the diff as a PR comment — split out because the pull_request job's token is read-only for fork PRs.
  • This is additive: the existing benchmarks job (CodSpeedHQ-reported history on main, with the existing environment-match guard) is untouched.

Known scope limits (see the module docstring in scripts/codspeed-local-diff.py):

  • Diffs are per [[bench]] target (rustpython/execution, rustpython/microbenchmarks, rustpython-sre_engine/benches), not per individual benchmark inside a target.
  • The instruction counts are not comparable to CodSpeed's own dashboard numbers (different tool invocation), only the base-vs-head ratio computed here matters.

Test plan

  • actionlint on both workflow files
  • Unit-verified the callgrind-output parser and diff renderer against synthetic data (no Valgrind available in the dev sandbox to run the measure subcommand end-to-end)
  • Confirm on a real PR run that results/head/results.json and results/base/results.json populate all three targets with non-zero instruction counts
  • Confirm the PR comment is posted/updated correctly, including from a fork PR

🤖 Generated with Claude Code

https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs

CodSpeed's own comparison is cross-run: base and head can each draw a
different GitHub Actions machine, and glibc's IFUNC-selected memcpy/malloc
implementations make even Valgrind/Callgrind instruction counts move with
which CPU was drawn (see https://codspeed.io/blog/why-glibc-faster-github-actions).
That can report a regression that is really just base and head having
landed on different hardware.

Add a `local-diff` job that measures base and head back-to-back in one PR
job (same runner, same glibc, same kernel) with a stock Valgrind wrapped
directly around the built `[[bench]]` targets, then posts the per-target
instruction-count diff as a PR comment. The comment is posted by a separate
`workflow_run`-triggered workflow, since the `pull_request` job's token is
read-only for fork PRs.

This is a per-bench-target ratio (not per individual benchmark, and not
comparable to CodSpeed's own dashboard numbers) alongside the existing
CodSpeedHQ-reported history on main, not a replacement for it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot added the z-ca-2026 Tag to track Contribution Academy 2026 label Sep 6, 2026
Comment on lines +1 to +4
on:
workflow_run:
workflows: [CodSpeed]
types: [completed]
continue-on-error: true # the artifact is absent on push/merge_group runs

- name: Post or update the PR comment
uses: actions/github-script@d746ffe35508b1917358783b479e04febd2b8f71 # v9.0.0
moreal and others added 6 commits September 6, 2026 18:36
cargo-codspeed names this directory after its internal BuildMode, not the
--measurement-mode flag: `simulation` (and `memory`) both build in
BuildMode::Analysis, which lands under target/codspeed/analysis/, not
target/codspeed/simulation/. Confirmed by a failed run on my fork's draft
PR (#25): the build step reported "Built 3 benchmark
suite(s)" but the measure step found none.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
Each bench binary is run with its package's manifest directory as cwd (to
match cargo-codspeed's own invocation), so a relative --out directory
resolved differently per package: `rustpython`'s manifest dir is the repo
root, so `results/head` happened to work there, but
`rustpython-sre_engine`'s manifest dir is crates/sre_engine, where the same
relative path doesn't exist -- valgrind failed with "can not open cache
simulation output file .../crates/sre_engine/results/head/...".

Confirmed against #25: rustpython/execution and
rustpython/microbenchmarks measured successfully, only sre_engine/benches
failed on this.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
The base commit predates this script's own addition to the repo, so
`git checkout <base sha>` removes it from the working tree entirely --
"Measure base" then failed with "can't open file
.../scripts/codspeed-local-diff.py: No such file or directory" (confirmed
on #25). Copy it to $RUNNER_TEMP once, before either
checkout, and invoke that copy for every step instead.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
--instr-atstart=no (what CodSpeed's own runner uses) leaves instrumentation
off until the instrument-hooks native library baked into the bench binary
calls CALLGRIND_START/STOP_INSTRUMENTATION around each benchmark -- but that
library only does so once CodSpeed's runner has set it up with env vars this
script doesn't replicate. Confirmed on #25: the run
completed end to end, `cargo-codspeed`'s harness printed a "Measured: ..."
line for every benchmark as usual, but every target's instruction count came
back exactly 0.

Instrument the whole process from start instead -- it doesn't depend on that
hook engaging, at the cost of also counting process startup/teardown, which
is negligible relative to running the full Python benchmark suite under
Callgrind and washes out in the base-vs-head ratio either way.

Also turn a 0 instruction count into a hard failure instead of a silently
wrong data point, since it's far more likely to mean the `Ir` column wasn't
found than that a benchmark genuinely executed zero instructions.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
Removing --instr-atstart=no didn't fix the 0-instruction-count failure on
#25, so the parser itself is the suspect now, not
instrumentation never turning on. Print the head/tail of the offending
.out file directly in the job log instead of taking another round trip
just to see it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
The file preview added last commit showed every dump landing as exactly
`summary: 0` on #25 -- Valgrind genuinely ran the whole
benchmark suite (confirmed by wall-clock differences and cargo-codspeed's
own per-benchmark "Measured: ..." log lines), but the `instrument-hooks`
native library linked into the bench binary never made a single
CALLGRIND_START/STOP_INSTRUMENTATION client request, so nothing was ever
counted regardless of --instr-atstart.

CodSpeedHQ/codspeed's own `get_base_injected_env` (src/executor/helpers/env.rs)
shows why: instrument-hooks only engages once it sees CODSPEED_ENV=runner,
CODSPEED_RUNNER_MODE=instrumentation and CODSPEED_PROFILE_FOLDER set. Set
those three (plus PYTHONHASHSEED=0, also set there) and restore
--instr-atstart=no now that the hooks will actually toggle it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
@codspeed-hq

codspeed-hq Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will degrade performance by 20.37%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 64 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Benchmark BASE HEAD Efficiency
gc_collect.py[rustpython] 127.1 ms 234.7 ms -45.82%
gc_traversal.py[rustpython] 854.1 ms 729.9 ms +17.02%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing moreal:codspeed-local-base-head-diff (b135ff5) with main (54ea5f5)

Open in CodSpeed

moreal and others added 6 commits September 6, 2026 22:37
Reading CodSpeedHQ/codspeed's own source (instrument-hooks/dist/core.c)
explains the persistent `summary: 0`: the bench binary's instrument-hooks
library doesn't make CALLGRIND_START/STOP_INSTRUMENTATION client requests
directly. On init it first tries to open a FIFO pair
(/tmp/runner.ctl.fifo, /tmp/runner.ack.fifo) and speak a versioned protocol
to whatever process is listening on the other end -- only the real
`codspeed` runner does that, so under a bare `valgrind --tool=callgrind`
wrap the FIFO handshake fails, the library falls back to doing nothing at
all, and every dump comes back empty regardless of --instr-atstart or the
CODSPEED_ENV/CODSPEED_RUNNER_MODE env vars (both tried and failed on
#25).

Shell out to `codspeed run --mode=simulation --skip-upload` (installed the
same way CodSpeedHQ/action does) instead of invoking valgrind ourselves --
it implements that protocol, so instrumentation actually engages. Nothing
is uploaded anywhere; the resulting .out files are parsed exactly as
before.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
…ines

Switching to codspeed run got real instrumentation going -- the .out file
went from a few hundred bytes to 111MB, with 90 events:/summary: dump pairs
matching every "Measured: ..." benchmark -- but every single summary: line
still read exactly 0, which cannot be right for a file that size. The
per-dump summary annotation instrument-hooks writes isn't a reliable total
under this many dumps; callgrind_annotate (shipped with Valgrind) computes
it the way any real consumer of the format does, by summing the actual
per-line cost records, and reports it as a PROGRAM TOTALS row.

Install plain apt valgrind alongside the codspeed CLI's own patched build,
purely for callgrind_annotate.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
The previous commit's failure ("no PROGRAM TOTALS line") gave no visibility
into what callgrind_annotate actually printed instead, so there's no way to
tell yet whether the totals row uses different wording in this version, or
callgrind_annotate is failing/warning some other way. Fold stdout/stderr
into the raised error so the next run's log shows it directly.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
…label

The stdout/stderr this now prints on failure showed exactly what was
missing: the totals row callgrind_annotate 3.26.0.codspeed7's output
actually produced was "... PROGRAM TOTALS (calculated)", not the bare
"PROGRAM TOTALS" the exact endswith() check required -- while the row
itself carried a perfectly plausible instruction count (~22 billion, for
the whole execution.rs suite). The many "line N malformed, ignoring"
warnings on stderr are apt's older callgrind_annotate not recognizing a few
of the patched Valgrind's newer record kinds (cfni=, extra event columns);
harmless, since it still tallies every cost line it does understand into
the same totals row.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
Confirmed on #25: with the totals label matched, the very
next attempt failed differently -- "invalid literal for int() with base 10:
'.'" -- because the apt-packaged callgrind_annotate is a version behind the
patched Valgrind that wrote the file and logs "line N malformed, ignoring"
for record kinds it doesn't recognize (cfni=, extra event columns). Having
skipped those, it can print a second, degenerate PROGRAM TOTALS row with an
unparseable placeholder instead of a real number, and the first commit's
fix took whichever row came first in the output.

Collect every parseable PROGRAM TOTALS candidate and take the largest --
skip anything that isn't a clean number rather than crashing on it, and a
real total for a whole benchmark suite run under Callgrind is far larger
than any garbage row a partial parse could produce.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
Confirmed on #25: codspeed run executes the bench binary
through a tiny bash wrapper script under --trace-children=yes, so a
target's directory holds one trivial .out file for that wrapper process
(Trigger: Program termination, Profiled target: /usr/bin/bash ...) besides
the real one for the spawned binary. The wrapper collects no cost records
at all, so callgrind_annotate renders its PROGRAM TOTALS row as literal `.`
placeholders in every column -- an expected empty file, which the previous
commit's "no parseable candidate" check treated as a hard failure instead.

Split the two failure modes: no PROGRAM TOTALS line at all is still a real
error, but one whose only numbers are unparseable placeholders now
contributes 0.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGKVKTJfvtVeUCnCNCtCBs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants