Repository navigation
feat: run each analysis benchmark in its own process - #122
not-matthias wants to merge 7 commits into
Conversation
Merging this PR will degrade performance by 86.96%3 warnings, 1 note
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
8728bb6 to
fdf9f67
Compare
--predictable and --predictable-gc-schedule fix the heap growing schedule but V8 still posts incremental marking steps as tasks on the event loop. Async work before a benchmark (Vitest's ESM loading, an awaited readFile loop) runs a varying number of those tasks, so when marking finishes, how many GCs ran and where objects end up differ between identical runs. Under Valgrind this shows as steps in both instruction counts (lazy sweeping inside the measured call) and cache misses. --no-incremental-marking-task makes marking progress only on allocation, which is deterministic. The flag exists on every supported Node major (22, 24, 26).
In analysis mode, runSync passed an async lambda to optimizeFunctionSync. Each warmup call returned at its first await, so its afterEach hook ran after the measured call instead of right after the warmup call, and the measured call ran with every warmup's state still live. The lambda is now synchronous, like the function runSync accepts.
The Vitest 5 provider only awaited functions declared async. A function that returns a promise without being async was run on the sync path, so its asynchronous work ran after the measured window closed and after afterEach. The provider now calls a non-async function once between its hooks, like tinybench does, and takes the async path when it returns a thenable. beforeAll is awaited up front for both paths.
tinybench calls a function that isn't declared async once in bench.add to tell whether it returns a promise. The walltime root-frame wrapper is sync, so every async benchmark was probed this way, and with forks every process re-registered and probed every benchmark in the file. Benchmarks with side effects ran in forks that never measure them.
A process now registers an async no-op for benchmarks it doesn't run. For the benchmarks it runs, an async function is registered with { async: true } so tinybench 6 skips the probe; the wrapper stays sync because an async wrapper adds an await to every iteration.
fdf9f67 to
675ecac
Compare
675ecac to
9bd3d5a
Compare
 [Medium impact] The changes appear safe to merge; no blocking issue was found. SummaryAnalysis benchmarks now run in separate processes in tinybench and Vitest 5.
No actionable issues were found in the changes since the previous review or the full-PR rule check. No previous review threads were supplied. This review used source checks; no runtime tests were run.
|
9bd3d5a to
6cc3868
Compare
Benchmarks measured in one process share allocator, JIT, and heap state, so a benchmark's simulation or memory result depends on the ones that ran before it. Walltime runs already run every benchmark in fresh processes. Add `analysisIsolation()`, which reads the same environment as `walltimeIsolation()` but always uses one fork per benchmark: analysis measures a benchmark once, and `CODSPEED_WALLTIME_FORKS` does not apply. `CODSPEED_PROCESS_ISOLATION=false` keeps every benchmark in one process. Analysis forks reuse `runForks`: the instruments hold the measurement, so a fork writes empty samples, which tell the parent it measured its benchmark. Refs COD-4156
In simulation and memory mode, the parent now runs each task in one fresh process, which measures only that task. Children keep quiet about the banner and summary, like walltime forks. The in-process integration test mocks the core, which forks cannot see, so it turns isolation off. Refs COD-4156
With Vitest 5, simulation and memory runs now reuse the walltime fork flow: workers queue each benchmark instead of measuring it, and the main process runs one fresh `vitest bench` per benchmark once the run is over. The fork is restricted to the benchmark's file and measures only that benchmark. `CODSPEED_WALLTIME_FORKS` and the `forks` options do not apply: analysis measures a benchmark once. Vitest 3/4 still don't fork. Closes COD-4156
6cc3868 to
7d16c57
Compare
adriencaccia
left a comment
adriencaccia
left a comment
There was a problem hiding this comment.
Main question: does this need #123 to ship? Without it, tinybench results here move ×2 between two runs of the same commit. Isolation on by default is fine, with a follow-up to lower the startup cost.
| const rootCallingFile = getCallingFile(1); | ||
| const instrumentMode = getInstrumentMode(); | ||
| const isolationMode = getIsolationMode(instrumentMode, options); | ||
| const isolationMode = processIsolation(options.forks); |
There was a problem hiding this comment.
The description says this is stacked on #123, but the base is #124's branch and #123 isn't in it. Do we need to merge #123 to release this as well? Without it, every tinybench benchmark here pays the first-benchmark setup #123 removes (Memory 656 B → 34 KB, switch 2 23.5 µs → 661.5 µs), and that cost isn't stable on CI: two runs of 9bd3d5a give test_recursive_fibo_10 339 µs then 679 µs, two runs of 675ecac move 6 tinybench benchmarks by ×2. If so, can you restack on #123 and show two runs of the same commit agree?
https://app.codspeed.io/CodSpeedHQ/codspeed-node/runs/compare/6af1f0830bbd4d5a590423c6..6af1f0950bbd4d5a590423cd
https://app.codspeed.io/CodSpeedHQ/codspeed-node/runs/compare/6af1d65b42a1644c0fdb1080..6af1d66842a1644c0fdb1083
| const analysis = getInstrumentMode() === "analysis"; | ||
| return { kind: "Forks", forks: analysis ? 1 : resolveWalltimeForks(forks) }; |
There was a problem hiding this comment.
One process per analysis benchmark costs a full Node or Vitest startup under Valgrind. On CI, from #124 to this PR: tinybench simulation 4m26 → 8m20 (+6.7 s per benchmark), memory 55 s → 1m52; Vitest simulation 6m20 → 11m43 (+14 s per benchmark), memory 1m07 → 2m48. A 100-benchmark Vitest suite would run ~30 min longer. We can ship it like this and follow up to lower the overhead. For the final release, it would be great to have numbers showing the variance improvement, to understand the tradeoff.
https://github.com/CodSpeedHQ/codspeed-node/actions/runs/42632341179
https://github.com/CodSpeedHQ/codspeed-node/actions/runs/42676098172
| // tinybench calls a function that isn't `async` once when it's added, to | ||
| // tell whether it returns a promise. A process that doesn't run the task | ||
| // registers a stub instead, so the user's function isn't called there. | ||
| if (!runsTaskFn(isolationMode, uri)) { |
There was a problem hiding this comment.
runsTaskFn now decides for analysis too: move it from walltime.ts to shared.ts.
GuillaumeLagrange
left a comment
GuillaumeLagrange
left a comment
There was a problem hiding this comment.
I think unless we have actual data about how it improves variance, we don't want to pay the startup under valgrind cost, and if we do we should definitely try and take mitigation actions to not pay the full price. We can discuss this deeper if you will
| * Process isolation: every benchmark runs in fresh copies of the benchmark | ||
| * process (forks), so the allocator state, JIT state, and handles that other | ||
| * benchmarks or setup code leave behind can't change its measurement. Walltime | ||
| * rounds within one process are correlated (JIT tier-up, heap sizing), so the | ||
| * rounds of several forks are pooled into one result. Analysis runs measure | ||
| * each benchmark once, in one fork. |
There was a problem hiding this comment.
let's format this a bit better to be more readable.
Also the comment's first sentence hints at this process isolation being on no matter what.
| ); | ||
| } | ||
| return { kind: "Forks", forks: resolveWalltimeForks(forks) }; | ||
| const analysis = getInstrumentMode() === "analysis"; |
There was a problem hiding this comment.
nit: isAnalysis or soemthing that hints at it being a boolean
TLDR: Simulation and memory runs measured every benchmark in one shared process, so allocator, JIT and heap state from earlier benchmarks leaked into later ones. Each analysis benchmark now runs in its own fresh process, reusing the walltime fork machinery (tinybench and Vitest 5).
core:analysisIsolation()reads the same env aswalltimeIsolation(), but always uses 1 fork.CODSPEED_WALLTIME_FORKSandforksoptions don't apply.CODSPEED_PROCESS_ISOLATION=falserestores the shared process.tinybench: the parent runs each task viarunForks(uri, 1); the child measures only that task.vitest(v5): workers queue analysis benchmarks; the main process runs onevitest benchper benchmark after the run. Vitest 3/4 and benchmark.js are unchanged.valgrind --tool=callgrind --trace-children=yes(runner args),examples/with-typescript-simple-esmandexamples/with-vitest-v5produce one dump per benchmark, each from a distinct PID, none from the parent.switch 1/switch 2measure 4,625/4,683 Ir (shared process: 4,625/3,152).Review notes
Closes COD-4156