ci: make the CodSpeed memory benchmarks deterministic - #298
Conversation
The memory job ran without the V8 flags CodSpeed requires for its
analysis modes; its log says so on every run ("[CodSpeed] missing
required flags: ... --no-opt, --predictable, ..."). Without them V8
optimizes on background threads, and TurboFan's zone allocations land in
whichever benchmark is being measured, so the same code reported swings
of up to several MB between runs. That is what produced the memory
"regressions" on pull requests that only touched tests.
Measured locally by counting native allocations inside each measured
window (1 vs 4 CPUs, 3 runs each): without the flags every one of the
38 memory benchmarks varied, with about 3,500 TurboFan zone segments
landing in the windows; with them, none do, and the large benchmarks
agree within ~0.3%.
- `benchmark:memory` passes the same flags as `benchmark`.
- The benchmark jobs pin `ubuntu-24.04` and Node.js 24.21.0, the
environment `ubuntu-latest` and `lts/*` resolve to today, so base and
head runs no longer straddle an image or Node.js update (Node.js 26
becomes `lts/*` in October).
- The wrapper logs the actual CodSpeed mode instead of always
"simulation".
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F
|
One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via: Supported
Alternatively, if the co-author should not be included, remove the Please update your commit message(s) by doing |
Node.js updates can make the library faster or leaner, and the dashboard should show that, so the benchmarks keep following `lts/*`. Only the runner image stays pinned. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughThe benchmark and memory-benchmark jobs now use the pinned Ubuntu 24.04 runner image. The memory benchmark script uses deterministic V8 flags. The CodSpeed wrapper instruments memory mode and logs the selected mode. It performs pre-measurement garbage collection only in memory mode. The local-runner documentation describes the required flags and their effect on memory results. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is evident; the benchmark changes are ready for normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
|
EasyCLA fails on this PR because of the Generated by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #298 +/- ##
=======================================
Coverage 99.10% 99.10%
=======================================
Files 27 27
Lines 6337 6337
Branches 812 812
=======================================
Hits 6280 6280
Misses 56 56
Partials 1 1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will regress 11 benchmarks
|
The CodSpeed wrapper ran gc() twice right before every measured call. Memory mode needs that, so warmup garbage is not counted, but in simulation mode it puts the heap's re-setup into the measured call: under callgrind, raw-source benchmarks spend 35-60k more instructions with it (30-50% of the short ones, e.g. isBuffer() 198,960 vs 144,525), and on CodSpeed that is the part that moved between runs of identical code, e.g. the one RawSource constructor in isBuffer() measured 114 us in one run and 211 us, with syscalls, in the next. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F
Measuring right after gc() (main) puts the heap's re-setup into the
measured call; not collecting at all (previous commit) lets garbage from
earlier tasks trigger a collection inside it, so a task's result
depends on the tasks that ran before it (concat-source buffers() nested
went 326k -> 768k instructions, 1.7ms on CodSpeed). Collect, then run
the body once more before measuring.
Under callgrind, full cases suite (174 tasks):
run-to-run depends on preceding suites
gc before measuring 1.18% 14/39 tasks >1%, max 46%
no gc 0.26% 27/39 tasks >1%, max 171%
gc + one more run 0.07% 7/39 tasks >1%
Memory mode is unchanged: it collects right before the measured call.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F
The two previous commits changed when simulation mode collects garbage. Locally under callgrind the last variant was the most stable, but on CodSpeed it made short benchmarks much slower (new RawSource(buffer) 70 us -> 938 us, flame graph dominated by runtime internals), which the local runs do not reproduce. Restore main's behavior so this pull request only carries the verified memory-mode fix; the simulation-mode variance needs its own investigation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F
|
CodSpeed Performance Analysis is red against The two CodSpeed runs on this PR show whether the numbers are now stable.
For comparison, the #297 run against its base showed 16 memory regressions of up to 90%, including benchmarks in the MB range. A maintainer needs to acknowledge this report on CodSpeed; I can't do that from here. Generated by Claude Code |
2 participants
Summary
CodSpeed's memory benchmarks report different numbers for the same code. On #297, which only changed tests, CodSpeed flagged 16 memory regressions of up to -90%.
Cause: the memory job ran without the V8 flags CodSpeed requires for its analysis modes. Every run's log said:
Without these flags, V8 optimizes on background threads, and TurboFan's zone allocations land in whichever benchmark is being measured at the time.
Evidence (local)
I counted native allocations inside each measured window, with CodSpeed's start/stop hooks marking the window. Each configuration ran 3 times on 1 CPU and 3 times on 4 CPUs:
replace-source: source()576 B – 345 KB,original-source: map({ columns: true })1.86 – 2.27 MBWithout the flags, backtraces show about 3,500 TurboFan zone segments inside the measured windows (
OptimizingCompileDispatcher::CompileTask→Zone::Expand). With the flags there are none. On this PR, the memory job no longer prints the warning.Changes
benchmark:memorypasses the same V8 flags asbenchmark.ubuntu-24.04, which is whatubuntu-latestresolves to today. A base run and a head run on either side of an image update would otherwise shift every benchmark at once. Node.js stays onlts/*, so Node.js improvements show up on the dashboard.benchmark:memoryneeds the flags.Not in this PR: simulation-mode variance
CPU (simulation) benchmarks also moved by up to ±35% between two
maincommits that only changed tests. The short ones are dominated by harness overhead: thegc()right before each measured call, plus the call wrapper. I tried two changes to when thatgc()runs. Both were deterministic locally under callgrind, but on CodSpeed they made short benchmarks slower (for examplenew RawSource(buffer)went from 70 µs to 938 µs, with a flame graph dominated by runtime internals), and my local runs don't reproduce that. Both are reverted here; this needs a separate investigation.Expected CodSpeed result on this PR
main's memory numbers were measured without the flags, so this PR's report shows large memory changes in both directions once. Stability shows up in the runs after this one.🤖 Generated with Claude Code
https://claude.ai/code/session_01ThsbPWvW5vnYWEaFs2Q22F