Skip to content

fix: make simulation GC deterministic after async setup and fix bench function calls - #124

Open
not-matthias wants to merge 4 commits into
mainfrom
cod-4179-fix-nondeterministic-gc-and-async-bench-handling-in
Open

not-matthias wants to merge 4 commits into
mainfrom
cod-4179-fix-nondeterministic-gc-and-async-bench-handling-in

Conversation

@not-matthias

Copy link
Copy Markdown
Member

TLDR: Simulation results of benchmarks behind async setup (Vitest module loading, awaited I/O) change between identical runs, because V8 still runs incremental-marking steps as event-loop tasks under --predictable. This PR turns those tasks off and fixes three bugs in how the tinybench and Vitest 5 integrations call benchmark functions.

GC determinism (core)

  • Add --no-incremental-marking-task to the analysis V8 flags. Marking then progresses only on allocation.
  • Vitest 4 repro, 20 runs each under Valgrind: spread of the simulated cost 0.774% → 0.003% (traverse), 0.143% → 0.003% (alloc), 0.050% → 0.000% (lodash).
  • Native Node 24, tinybench after an awaited readFile loop: GCs before the measured call were 51 or 52, now 48 in 40/40 runs.

Per-run cost, Vitest 4

Run-to-run spread

Native GC count

Benchmark function calls (each has a regression test that fails on main)

  • tinybench runSync, simulation: the warmup lambda was async, so each warmup's afterEach ran after the measured call.
  • Vitest 5, simulation: a function that returns a promise without being async ran on the sync path, so its async work ran outside the measured window.
  • tinybench walltime forks: every fork called every async benchmark of the file once in bench.add. A fork now calls only its own benchmark.

Review notes

  • Costs shift once, e.g. Vitest 4 traverse −0.9%: marking work no longer lands in the measured call.
  • Vitest 5, simulation: sync functions get one more call before warmup, to detect a returned promise.
  • Vitest 5 alloc has a 1.2% step in 2/16 runs with this PR (0.065% spread before). It is the same without the flag and --no-flush-bytecode doesn't remove it. Natively, the GC count and young-generation usage at the measured call are identical in 24/24 runs, so the step is Valgrind-only and not attributed yet.
  • --no-flush-bytecode was evaluated and isn't added.
  • The runner's Node wrapper keeps its own copy of the analysis flags and needs the same flag.
  • Conflicts with fix: keep one-time setup costs out of the first measured benchmark #123 in packages/tinybench-plugin/src/analysis.ts (same runSync lambda); the resolution keeps both changes.

Closes COD-4179

--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.
@greptile-apps

greptile-apps Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no new actionable defects were found.

Summary

Makes simulation GC scheduling more repeatable and fixes benchmark call ordering.

  • Simulation runs keep incremental marking off the event loop.
  • Tinybench sync runs finish each hook before the next call.
  • Vitest 5 awaits benchmark functions that return promises.
  • Each tinybench walltime fork calls only its assigned benchmark.

Acknowledged by not-matthias: sync Vitest 5 functions receive an extra detection call; the runner’s separate flag list needs updating; the remaining Valgrind-only cost variation is not fixed here.

Reviews (1) · Last reviewed commit: "fix(tinybench): only call a benchmark in..." · Reviewed by Greptile

@codspeed

codspeed Bot commented Nov 9, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 10.78%

4 warnings, 1 note

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

⚠️ 13 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

⚠️ 9 benchmarks with a significant change ran on different hardware

Their base and head results were measured on machines with a different CPU, core count or memory, so the changes might not come from the code. Run base and head on the same runner type.

Learn more about Macro Runners

See affected benchmarks

⚠️ 14 benchmarks with a significant change ran in a different software environment

Their base and head results were measured with a different OS or toolchain, such as compiler, runtime or library versions, so the changes might come from this difference rather than from the code. Open an affected benchmark to see what changed.

See affected benchmarks

ℹ️ 16 benchmarks have no flamegraph

The flamegraph wasn't generated: most of the time was spent in runtime internals, which would leave nothing useful to analyze.

See affected benchmarks

⚡ 7 improved benchmarks
❌ 7 regressed benchmarks
✅ 256 untouched benchmarks
⏩ 56 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Memory test_recursive_fibo_20 1.3 KB 13.8 KB -90.7%
❌ CPU Simulation wait 500ms 29.9 ms 38.7 ms -22.8%
❌ Memory wait 1sec 22 KB 28 KB -21.4%
❌ CPU Simulation wait 1sec 61.3 ms 72.9 ms -15.86%
❌ Walltime test_iterative_fibo_10 84 ns 96 ns -12.5%
❌ CPU Simulation wait 1sec 53.7 ms 60.2 ms -10.78%
❌ CPU Simulation wait 500ms 26.6 ms 29.7 ms -10.69%
⚡ CPU Simulation switch 1 663.1 µs 313.9 µs ×2.1
⚡ Memory wait 1sec 28.7 KB 22.7 KB +26.5%
⚡ Memory wait 500ms 28.7 KB 22.7 KB +26.5%
⚡ Walltime switch 1 84 ns 72 ns +16.67%
⚡ Walltime switch 2 84 ns 72 ns +16.67%
⚡ Walltime test_recursive_cached_fibo_10 684 ns 588 ns +16.33%
⚡ Walltime test sync baz 10 96 ns 84 ns +14.29%

Tip

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


Comparing cod-4179-fix-nondeterministic-gc-and-async-bench-handling-in (16decfc) with main (442e921)

Open in CodSpeed

Footnotes

  1. 56 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. ↩

@not-matthias
not-matthias requested review from GuillaumeLagrange and adriencaccia and removed request for adriencaccia November 9, 2026 14:39
@not-matthias
not-matthias added this pull request to stack #125 November 9, 2026 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant