Add Vitest performance benchmarks with CI regression checks - #1601
Conversation
Introduces bench suites for generators, renderers, and zoom, plus a baseline workflow and PR check script that compares runs against a committed baseline and comments the diff.
There was a problem hiding this comment.
🟡 Changes recommended
CI stability/correctness issues are present in the benchmarks and workflows (node vs jsdom runtime error, nondeterministic benchmark inputs, and fork-PR commenting permissions), which can break or flake the regression checks.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a Vitest benchmark suite and CI automation to track performance over time, aiming to catch regressions (and highlight improvements) in key generator and renderer hotspots.
Changes:
- Adds multiple
*.bench.tsbenchmark suites covering generators, renderers, and zoom/viewport reconciliation behavior. - Adds a benchmark comparison script (
scripts/bench-check.js) plus committed baseline data (bench/baseline.json) and local-run artifacts in.gitignore. - Adds GitHub Actions workflows to run benchmark checks on PRs and to regenerate/update baselines on
master.
File summaries
| File | Description |
|---|---|
| src/renderers/viewport/viewport-renderer.bench.ts | Benchmarks ViewportRenderer reconcile behavior across a gesture (pre/post #1599 shape) |
| src/renderers/draw-rivers.bench.ts | Adds drawRivers benchmarks with synthetic river networks |
| src/renderers/draw-heightmap.bench.ts | Adds drawHeightmap benchmarks with synthetic Voronoi grid + height bands |
| src/renderers/draw-borders.bench.ts | Adds drawBorders benchmarks with synthetic state/province partitioning |
| src/generators/voronoi.bench.ts | Adds Voronoi construction benchmarks at multiple point counts |
| src/generators/states-generator.bench.ts | Adds States.expandStates benchmarks using a synthetic grid pack and heap impl |
| src/generators/routes-generator.bench.ts | Adds Routes.generate benchmarks using a synthetic pack and heap impl |
| src/generators/heightmap-generator.bench.ts | Adds HeightmapGenerator.fromTemplate benchmarks over increasing grid sizes |
| src/generators/cultures-generator.bench.ts | Adds Cultures.expand benchmarks with synthetic pack inputs |
| src/generators/burgs-generator.bench.ts | Adds Burgs.generate placement benchmarks with synthetic fully-populated packs |
| src/generators/biomes-generator.bench.ts | Adds Biomes.define benchmarks scaling with cell count |
| src/components/zoom.bench.ts | Benchmarks full zoom drag gesture path through applyZoomBehavior + ViewportRenderer |
| scripts/bench-check.js | Compares current vs baseline benchmark JSON and emits a PR-comment markdown summary |
| package.json | Adds bench, bench:baseline, bench:check npm scripts |
| bench/baseline.json | Adds committed benchmark baseline used for CI comparisons |
| .gitignore | Ignores generated bench/current.json and bench/comment.md artifacts |
| .github/workflows/deploy.yml | Avoids triggering deploy on baseline-only updates |
| .github/workflows/bench.yml | Runs benchmark checks on PRs and posts a sticky PR comment with results |
| .github/workflows/bench-baseline.yml | Regenerates and commits updated benchmark baselines on pushes to master |
Review details
- Files reviewed: 18/19 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Strip local absolute filepaths (and username) from bench/baseline.json by normalizing to repo-relative paths after each vitest --outputJson run. - Seed voronoi.bench.ts point generation deterministically instead of Math.random(), so baseline vs PR comparisons aren't flaky. - Run viewport-renderer.bench.ts under jsdom since ViewportRenderer touches `document`, which throws under the default node test environment. - Skip the sticky PR comment step in bench.yml on fork PRs, where GITHUB_TOKEN lacks pull-requests: write permission.
- zoom.bench.ts: wrap vi.useFakeTimers/useRealTimers calls so the bench hook returns void; tsc rejected the VitestUtils return type otherwise, breaking `npm run build`. - bench.yml: upload bench/current.json as an artifact so a CI-generated baseline can be pulled down. The committed baseline was measured on a laptop, which runs faster than the GitHub Actions runner, so every benchmark showed as a false regression on CI.
✅ Deploy Preview for afmg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Gating the comment step on same-repo PRs meant fork PRs (the normal case for an open-source project) never got benchmark feedback. Use the standard secure pattern instead: bench.yml runs the untrusted PR code with read-only permissions and uploads results as an artifact; the new bench-comment.yml is triggered by workflow_run (runs in the base repo's trusted context) and posts the PR comment for every PR, fork or not.
The committed baseline was measured on a laptop, which is meaningfully faster than the ubuntu-latest CI runner used by bench.yml, so every benchmark showed as a false regression on CI. Replace it with results pulled from an actual CI run (job 32599117416) so future PR comparisons run on comparable hardware.
Two consecutive CI runs with no code changes showed several benchmarks swinging 40-48%, which is just noise from GitHub-hosted runners sharing CPUs with other jobs, not a real regression. Run each benchmark longer (1.5s instead of the 500ms default) to average out more of that noise, and raise the failure threshold from 40% to 75% so the check still catches real (multi-x) regressions without flaking on run-to-run noise.
|
Had a look at the bench failure before reviewing. It wasn't a regression: all 33 benchmarks were slower in that run (+4% to +48%, median ~30%), nothing faster. That's the runner, not the code. And it was the run on The catch with going to 75% is that the gate can then only catch roughly 2x regressions. A real 30% regression is indistinguishable from a noisy neighbour. I tried the alternative on my fork: benchmark base and head in the same job, alternating between them over N rounds, and compare median ratios. Both refs hit the same noisy neighbours, so the machine offset cancels instead of needing to be thresholded away. Ran it 5 times on 5 different runners with base == head, so every reported number is pure error:
So 25-30% looks usable, which would make the check about 3x more sensitive than 75%. Cost is ~4.5-6 min vs ~1 min, and it drops Script and workflow: https://github.com/barrulus/Fantasy-Map-Generator/tree/bench-ab-experiment ( Two smaller things: several benchmarks are sub-millisecond ( |
|
That's a great idea! |
Credit to @barrulus (Azgaar#1601 (comment)) for the idea and reference implementation. A committed baseline compares two different CI runs, so a busy neighbour on the shared runner shows up as a "regression" that has nothing to do with the code (see the false-positive runs earlier in this PR). Instead, benchmark base (master) and head (the PR) alternately in the same job across several rounds and compare median per-benchmark ratios — both refs hit the same noisy neighbours, so that noise cancels out instead of needing a threshold wide enough to hide it. Validated with 5 self-comparison runs (base == head) on different runners: median error 1.4%, p90 3.7%, worst single measurement 23.9%, zero false alarms at a 25% threshold across 165 measurements. That's roughly 3x more sensitive than the 75% threshold the stored-baseline approach needed to stay quiet. This also removes bench/baseline.json and bench-baseline.yml entirely: there's nothing to keep in sync anymore, so no more stale-baseline updates on every merge to master. Trade-off: each PR now benchmarks both refs across 3 rounds (~4.5-6 min) instead of one comparison against a stored baseline (~1 min).
Credit to @barrulus (Azgaar#1601 (comment)) for the idea and reference implementation. A committed baseline compares two different CI runs, so a busy neighbour on the shared runner shows up as a "regression" that has nothing to do with the code (see the false-positive runs earlier in this PR). Instead, benchmark base (master) and head (the PR) alternately in the same job across several rounds and compare median per-benchmark ratios — both refs hit the same noisy neighbours, so that noise cancels out instead of needing a threshold wide enough to hide it. Validated with 5 self-comparison runs (base == head) on different runners: median error 1.4%, p90 3.7%, worst single measurement 23.9%, zero false alarms at a 25% threshold across 165 measurements. That's roughly 3x more sensitive than the 75% threshold the stored-baseline approach needed to stay quiet. This also removes bench/baseline.json and bench-baseline.yml entirely: there's nothing to keep in sync anymore, so no more stale-baseline updates on every merge to master. Trade-off: each PR now benchmarks both refs across 3 rounds (~4.5-6 min) instead of one comparison against a stored baseline (~1 min). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
e03a95c to
d38e95e
Compare
writeFileSync(markdownOut, ...) threw ENOENT because bench/ was removed along with the old baseline file. This always triggers on this PR's own CI check, since origin/master has no bench suite yet to compare against and the script's "no comparable benchmarks" fallback tried to write its message to that same missing directory. Create parent directories before writing any output file.
|
Incoperated your idea @barrulus. thanks! |
|
I like the idea of the A/B testing a lot, but we need to agree on what we are measuring and on what data. So I will revert it from 1.148 for now and we can push it later directly to master. The suites run on hand-built unrealistic square lattices, where real I'd rather measure something closer to what matters:
We already have Playwright, cached builds, and |
|
I'd built just TWO Playwright specs and one script, all under
|
|
Prototyped the three files you described: https://github.com/barrulus/Fantasy-Map-Generator/tree/perf/playwright-ab (plus a small perf-only playwright config — the main one pins testDir to tests/e2e). generation.spec.ts regenerates at fixed seed/cell counts (10K and 100K), reading the existing TIME log from the console — no app changes, and it keeps working under your Pipeline sketch since that keeps The hash check caught something on its first run: regeneration is nondeterministic on master. First load with ?seed= reproduces 6/6, but New Map with the same seed gives one of two different maps (2:1 over 6 runs). Terrain, rivers and burg counts are identical between the variants; divergence starts at generateCultures. So something async left over from the previous map consumes seeded Math.random draws between the rivers and cultures stages — your per-step RNG locking would fix exactly this. User-visible as a seed found via New Map not always reproducing from a URL. Because of it the gate only fails when base and head are each self-consistent but differ; within-side nondeterminism is a warning. Can file it as a separate issue. Happy to open this as a PR if you want it as the base. The interaction metrics need base==head calibration on CI runners before any of them gate. |
|
Yes, sure, let have a new PR |
Introduces bench suites for generators, renderers, and zoom, plus a baseline workflow and PR check script that compares runs against a committed baseline and comments the diff.
I was inspired by #1599
The idea would be: for future performance improvements, we can put these benchmark checks in place (done with vitest benchmarks). Then we can merge them and the PR check will show if there was an improvement in the performance or even a decrease.
I also added some baselines for performance heavy code.
There were also 2 new github actions added:
bench.yml => runs on PRs to master and checks the benchmark against the baseline
bench-baseline.yml => runs on merge to master and updates the benchmark baselines to prevent stale checks