ci(codspeed): move the simulation bench gate to a dedicated self-hosted runner - #75
ci(codspeed): move the simulation bench gate to a dedicated self-hosted runner#75sedghi wants to merge 10 commits into
Conversation
…d runner Move the codspeed-bench job off GitHub's shared pool (ubuntu-24.04) onto a fixed self-hosted runner labeled 'codspeed-bench'. Simulation instruction counts derive from the runner CPU's cache model, and the shared pool randomly assigns Intel vs AMD hardware, producing spurious cross-run 'Different runtime environments' deltas. A dedicated fixed box keeps every run on identical hardware so the gate is stable. Adds docs/ci/self-hosted-runner.md with the box requirements and cutover order.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds a gated CodSpeed simulation workflow for the shared ChangesSelf-hosted CodSpeed benchmark workflow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PullRequest
participant BenchWorkflow
participant PRChecks
participant NashuaRunner
participant LockWrapper
participant CodSpeed
PullRequest->>BenchWorkflow: Trigger benchmark workflow
BenchWorkflow->>BenchWorkflow: Classify changes and select packages
BenchWorkflow->>PRChecks: Poll matching build artifacts
PRChecks-->>BenchWorkflow: Report build readiness
BenchWorkflow->>NashuaRunner: Start scoped benchmark job
NashuaRunner->>LockWrapper: Acquire shared filesystem lock
LockWrapper->>CodSpeed: Run pinned simulation benchmark
CodSpeed-->>LockWrapper: Return benchmark status
LockWrapper->>LockWrapper: Release lock
🚥 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 |
…ore is a throughput win
Merging this PR will degrade performance by 40.12%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decode CT-512x512-near-lossless.JLS (.81 near-lossless) — cold |
19.8 ms | 37.9 ms | -47.78% |
| ❌ | Simulation | decode CT-512x512-near-lossless.JLS (.81 near-lossless) — warm |
19.8 ms | 37.9 ms | -47.76% |
| ❌ | Simulation | HTJ2K Lossless (.201) |
141.4 ms | 179.7 ms | -21.3% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ci/codspeed-self-hosted (cb6757f) with main (a88a461)2
Footnotes
-
66 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. ↩
-
No successful run was found on
main(9c086c9) during the generation of this report, so a88a461 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
- flock: add tools/ci/with-nashua-lock.sh (same lock path as cornerstone3D and OHIF), wrapping the bench command inside the CodSpeed action's `run:`; plus timeout-minutes: 180 to bound the added lock wait. - node: pin both codspeed jobs to 22.23.1 — a range reuses whatever 22.x is in the box's persistent tool cache, and V8 patch releases move the numbers. - yarn: provision per-job via Corepack; nashua has none and setup-node installs node + npm only. - docs: document the shared-box mutex, and correct the valgrind requirement — CodSpeed installs its own patched build, pre-installed by hand and held here.
![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/pr-checks.yml:
- Line 484: Update the detect-changes TOOLCHAIN_PATHS configuration to include
tools/ci/with-nashua-lock.sh (or the tools/ci/ directory), so changes to the
lock wrapper set any=true and trigger the benchmark command using it.
In `@docs/ci/self-hosted-runner.md`:
- Around line 24-27: Update the supported OS list in the self-hosted runner
documentation to remove Debian 11, leaving Ubuntu 22.04/24.04 and Debian 12 as
the documented releases. Keep the surrounding architecture and CodSpeed
compatibility guidance unchanged.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6589db6b-8e7e-46cb-8c5e-ee4d0b38d965
📒 Files selected for processing (3)
.github/workflows/pr-checks.ymldocs/ci/self-hosted-runner.mdtools/ci/with-nashua-lock.sh
|
@sedghi - with this PR, we have a self hosted runner which becomes exposed: |
Tags can be moved upstream; the SHA freezes both the CI code and the instrumentation environment the baseline was measured under.
Adds CODEOWNERS routing /.github/ and /tools/ci/ to named owners; binds via the require_code_owner_reviews branch protection flag on main.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
docs/ci/self-hosted-runner.md (1)
3-3: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDocument the existing PR trust boundary for this runner.
.github/workflows/bench.ymlalready limits fork PR code paths to read-onlypull_requestactions, defers CI-defining changes unless the author has write access, and downloads PR-checks artifacts from the base workflow. Add that rule to this runner doc before registration so the shared runner use case is transparent.🤖 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 `@docs/ci/self-hosted-runner.md` at line 3, Update the self-hosted runner documentation before the registration instructions to describe the existing PR trust boundary enforced by the codspeed-bench workflow: fork PRs use read-only pull_request actions, CI-defining changes are deferred unless the author has write access, and PR-checks artifacts come from the base workflow.
🤖 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/bench.yml:
- Line 217: Add a .github/actionlint.yaml configuration declaring codspeed-bench
and nashua under self-hosted-runner.labels, so actionlint recognizes the custom
labels used by the runs-on declaration in the workflow.
- Around line 217-229: Harden the benchmark job gated by
needs.gate.outputs.proceed and needs.gate.outputs.ready so fork pull requests
cannot execute on the shared self-hosted runner; require the pull request head
repository to match github.repository, or use a protected environment with
required reviewers. Update actions/checkout@v4 to set persist-credentials:
false, and preserve the existing same-repository and non-pull-request execution
paths.
- Around line 295-358: Validate each package name in the scope-computation step
before appending it to flags, accepting only plain scoped package names and
rejecting invalid values such as newlines or shell metacharacters. Pass the
resulting scope flags to the CodSpeed action through an environment variable
rather than interpolating steps.scope.outputs.flags into the run command. Apply
the equivalent validation and environment-based flag passing to the matching
walltime benchmark flow.
---
Nitpick comments:
In `@docs/ci/self-hosted-runner.md`:
- Line 3: Update the self-hosted runner documentation before the registration
instructions to describe the existing PR trust boundary enforced by the
codspeed-bench workflow: fork PRs use read-only pull_request actions,
CI-defining changes are deferred unless the author has write access, and
PR-checks artifacts come from the base workflow.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 09965bbf-1d2b-45be-9e7a-f2fe4fd188fd
📒 Files selected for processing (4)
.github/CODEOWNERS.github/workflows/bench.yml.github/workflows/pr-checks.ymldocs/ci/self-hosted-runner.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/pr-checks.yml (1)
371-375: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPin the build Node.js version to the benchmark version.
codspeed-walltimeandcodspeed-benchbenchmark thedist-*artifacts produced by thebuildjob. Ifactions/setup-node@v4resolvesnode-version: '22'to a different Node 22 patch than22.23.1, CodSpeed can compare artifacts generated with different tooling. Set the build job to22.23.1too.Proposed fix
- node-version: '22' + node-version: '22.23.1'🤖 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 @.github/workflows/pr-checks.yml around lines 371 - 375, Update the build job’s actions/setup-node configuration to use Node.js version 22.23.1, matching the existing codspeed-bench pin and the benchmark jobs. Keep the artifact build flow unchanged.
🤖 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.
Outside diff comments:
In @.github/workflows/pr-checks.yml:
- Around line 371-375: Update the build job’s actions/setup-node configuration
to use Node.js version 22.23.1, matching the existing codspeed-bench pin and the
benchmark jobs. Keep the artifact build flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96061fe8-6d2b-4fac-ab71-e4df79596e1b
📒 Files selected for processing (2)
.github/workflows/bench.yml.github/workflows/pr-checks.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/bench.yml
3 participants
Why
CodSpeed simulation mode derives its modeled CPU cache from the physical runner CPU. GitHub's shared runners randomly assign Intel Xeon 8370C vs AMD EPYC 7763, so identical source shifts instruction counts run-to-run and CodSpeed flags "Different runtime environments detected" — the noise behind the phantom regressions we've been triaging. Pinning the OS wasn't enough; the CPU model is the remaining variable.
Change
codspeed-bench(the blocking simulation gate) now runs onruns-on: [self-hosted, codspeed-bench]instead ofubuntu-24.04. One fixed box → every baseline and PR run on identical hardware → Simulation is stable.docs/ci/self-hosted-runner.md: box requirements (valgrind, node 22, isolated/pinned CPU), how to register the runner, and the cutover order.build/test/dist-size/browser-smokestay on GitHub-hosted runners. (codspeed-walltimealready targetscodspeed-macrobehind a repo variable — unchanged.)Cutover order matters:
codspeed-benchlabel FIRST (see the doc). If this merges before a runner with that label exists, thecodspeed-benchjob queues indefinitely.mainpush re-seeds the CodSpeed baseline on the fixed hardware. The first PR comparisons after cutover show a one-time environment shift (old shared-runner baseline vs new fixed head) — expected, self-resolves once the baseline is re-seeded.Rollback: revert
runs-ontoubuntu-24.04.Summary by CodeRabbit
New Features
main, and manual runs.Chores
Documentation