Add real generation and interaction performance benchmarks with A/B comparison - #1608
Open
SheepFromHeaven wants to merge 11 commits into
Open
Add real generation and interaction performance benchmarks with A/B comparison#1608SheepFromHeaven wants to merge 11 commits into
SheepFromHeaven wants to merge 11 commits into
Conversation
Per maintainer feedback on PR Azgaar#1601: replaces synthetic bench.ts fixtures (hand-built square lattices unlike real Voronoi-generated maps) with Playwright specs measuring actual generation (fixed seeds, per-stage TIME instrumentation already in every generator) and actual interaction (scripted zoom/pan over a real fixture .map file). Compares base vs head by alternating rounds on the same runner, per the earlier finding that a stored baseline can't survive shared-runner noise.
Confirmed locally (HEAD vs HEAD, single round): stages under ~1ms are dominated by JIT/GC jitter and can swing 40-75% between two runs of the exact same code. total/gesture-level metrics (tens-hundreds of ms) stay stable and are what should actually gate the check. Report sub-2ms stages in the comment for visibility, but exclude them from the regression determination.
tests/fixtures/*.map gets new versioned fixtures over time; hardcoding 1.143.1.map would need a manual bump each time. Pick the highest version by parsing the X.Y.Z filename instead.
Same bootstrap gap as the vitest-bench version had: when the base ref has no tests/perf/playwright.config.ts yet (true for this PR's own master), the previous code let execFileSync throw uncaught instead of degrading to the existing "no comparable metrics" path. Also catch Playwright's own non-zero exit generally and parse whatever PERF_RESULT lines made it to stdout, rather than losing partial results to one failed test.
Replaces every TIME-guarded console.time/console.timeEnd pair with timeStart/timeEnd (src/utils/perfEvents.ts), which still logs to the console for devtools users but also dispatches a perf:stage CustomEvent with the stage name and duration. map:generated now carries totalMs too. generation.spec.ts listens for these events directly instead of regex-parsing console message text, which is more robust (the earlier console-format assumption already broke once) and gives other consumers (e.g. a future perf overlay) a real API instead of scraping devtools output.
✅ Deploy Preview for afmg ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The perf suite and comment workflow have a few correctness issues (Playwright test timeout, artifact-download permissions, and robustness of result parsing/event emission) that can cause CI failures or incomplete benchmark reporting.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a Playwright-based performance benchmarking suite that measures real map generation and scripted interactions, plus CI workflows to run an alternating A/B comparison between master and the PR head. It also replaces ad-hoc TIME && console.time* instrumentation with timeStart/timeEnd helpers that emit structured perf:stage events and extends map:generated with total generation time for reliable collection in tests.
Changes:
- Introduces perf specs (
generation+interaction) and a Node A/B harness (ab.mjs) that alternates base/head rounds and compares median metric ratios. - Adds
timeStart/timeEnd+perf:stageevent dispatch, and migrates manyTIME-guarded timings to use the helper. - Adds GitHub Actions workflows to run benchmarks on PRs and post results as a sticky PR comment via
workflow_run.
File summaries
| File | Description |
|---|---|
| tests/perf/playwright.config.ts | Playwright config for perf suite (single worker, fixed viewport, baseURL via env). |
| tests/perf/interaction.spec.ts | Interaction benchmark that loads latest .map fixture and measures zoom/pan gesture duration. |
| tests/perf/generation.spec.ts | Generation benchmark that records per-stage timings via perf:stage and total via map:generated. |
| tests/perf/ab.mjs | A/B harness that builds/serves base+head, alternates runs, parses PERF_RESULT, compares medians. |
| src/utils/perfEvents.ts | Adds timeStart/timeEnd helpers and perf:stage event emission. |
| src/utils/index.ts | Exposes timeStart/timeEnd on window and exports via utils barrel. |
| src/utils/graphUtils.ts | Switches timing instrumentation to timeStart/timeEnd. |
| src/services/io/export.ts | Switches export timings to timeStart/timeEnd. |
| src/services/io/export-json.ts | Switches JSON export timing to timeStart/timeEnd. |
| src/renderers/labels/labels-renderer.ts | Switches label draw timing to timeStart/timeEnd. |
| src/renderers/erosion-bake.ts | Switches erosion bake timing to timeStart/timeEnd. |
| src/renderers/draw-temperature.ts | Switches temperature render timing to timeStart/timeEnd. |
| src/renderers/draw-states.ts | Switches states render timing to timeStart/timeEnd. |
| src/renderers/draw-scalebar.ts | Switches scalebar render timing to timeStart/timeEnd (guarded on first render). |
| src/renderers/draw-routes.ts | Switches routes render timing to timeStart/timeEnd. |
| src/renderers/draw-rivers.ts | Switches rivers render timing to timeStart/timeEnd. |
| src/renderers/draw-religions.ts | Switches religions render timing to timeStart/timeEnd. |
| src/renderers/draw-relief-icons.ts | Switches relief render timing to timeStart/timeEnd. |
| src/renderers/draw-provinces.ts | Switches provinces render timing to timeStart/timeEnd. |
| src/renderers/draw-precipitation.ts | Switches precipitation render timing to timeStart/timeEnd. |
| src/renderers/draw-ocean.ts | Switches ocean render timing to timeStart/timeEnd. |
| src/renderers/draw-military.ts | Switches military render timing to timeStart/timeEnd. |
| src/renderers/draw-markets.ts | Switches markets render timing to timeStart/timeEnd. |
| src/renderers/draw-markers.ts | Switches markers render timing to timeStart/timeEnd. |
| src/renderers/draw-landmass.ts | Switches landmass render timing to timeStart/timeEnd. |
| src/renderers/draw-ice.ts | Switches ice render timing to timeStart/timeEnd. |
| src/renderers/draw-heightmap.ts | Switches heightmap render timing to timeStart/timeEnd. |
| src/renderers/draw-goods.ts | Switches goods render timing to timeStart/timeEnd. |
| src/renderers/draw-emblems.ts | Switches emblems render timing to timeStart/timeEnd including async redraw path. |
| src/renderers/draw-cultures.ts | Switches cultures render timing to timeStart/timeEnd. |
| src/renderers/draw-burg-icons.ts | Switches burg icons render timing to timeStart/timeEnd. |
| src/renderers/draw-borders.ts | Switches borders render timing to timeStart/timeEnd. |
| src/renderers/draw-biomes.ts | Switches biomes render timing to timeStart/timeEnd. |
| src/generators/zones-generator.ts | Switches zones generation timing to timeStart/timeEnd. |
| src/generators/states-generator.ts | Switches multiple states generator stage timings to timeStart/timeEnd. |
| src/generators/routes-generator.ts | Switches routes generation timings to timeStart/timeEnd. |
| src/generators/river-generator.ts | Switches river generation timing to timeStart/timeEnd. |
| src/generators/religions-generator.ts | Switches religions generation timing to timeStart/timeEnd. |
| src/generators/relief-generator.ts | Switches relief generation timing to timeStart/timeEnd. |
| src/generators/provinces-generator.ts | Switches provinces generation timing to timeStart/timeEnd. |
| src/generators/production-generator.ts | Switches production timing to timeStart/timeEnd. |
| src/generators/ocean-generator.ts | Switches ocean generation timing to timeStart/timeEnd. |
| src/generators/military-generator.ts | Switches military generation timing to timeStart/timeEnd. |
| src/generators/measurers-generator.ts | Switches default ruler creation timing to timeStart/timeEnd. |
| src/generators/markets-generator.ts | Switches markets generation timing to timeStart/timeEnd. |
| src/generators/markers-generator.ts | Switches marker generation timing to timeStart/timeEnd. |
| src/generators/heightmap-generator.ts | Switches heightmap definition timing to timeStart/timeEnd. |
| src/generators/goods-generator.ts | Switches goods generation/regeneration timing to timeStart/timeEnd. |
| src/generators/features.ts | Switches feature markup timings to timeStart/timeEnd. |
| src/generators/cultures-generator.ts | Switches cultures generation/expansion timings to timeStart/timeEnd. |
| src/generators/burgs-generator.ts | Switches burg generation/specification timings to timeStart/timeEnd. |
| src/generators/biomes-generator.ts | Switches biomes definition timing to timeStart/timeEnd. |
| src/controllers/heightmap-editor.ts | Switches heightmap editor regeneration/restore timings to timeStart/timeEnd. |
| public/main.js | Adds totalMs computation, uses timeStart/timeEnd for stages, extends map:generated detail with totalMs. |
| package.json | Adds perf:ab script entry. |
| .gitignore | Ignores generated perf-report/ outputs. |
| .github/workflows/perf.yml | Runs A/B perf benchmarks on PRs and uploads a comment artifact. |
| .github/workflows/perf-comment.yml | Posts perf results comment from workflow_run context using sticky PR comments. |
Review details
- Files reviewed: 57/58 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- tests/perf/playwright.config.ts: set an explicit 180s test/expect timeout so the 120s waits in the perf specs don't hit Playwright's default 30s timeout. - tests/perf/ab.mjs: skip malformed PERF_RESULT lines instead of letting JSON.parse abort the whole A/B run. - src/utils/perfEvents.ts: check for a matching timeStart before calling console.timeEnd (avoids a noisy "No such label" warning) and guard the perf:stage dispatch behind a window check. - public/main.js: only include totalMs in the map:generated detail when it's a number, since showStatistics() is also called from load/resample paths with no total to report. - .github/workflows/perf-comment.yml: add actions: read so actions/download-artifact can read the triggering run's artifacts.
…rktrees Adopts two ideas from barrulus's parallel implementation (barrulus:perf/playwright-ab): - generation.spec.ts now hashes the generated pack (cells, burgs) per seed via FNV-1a and reports it alongside timings. ab.mjs compares checksums across rounds: if a side disagrees with itself, that's a pre-existing determinism bug reported as a warning; if base and head are each internally consistent but differ from each other, that's a real correctness regression and fails the run regardless of timing. - ab.mjs copies the perf spec files from the head worktree into the base worktree before running, so both sides run identical measurement code. This also fixes the bootstrap gap where base (predating this PR) had no perf suite to compare against: base now runs head's specs against its own application code, so a real A/B comparison against master works even before this PR merges. - generation.spec.ts falls back to parsing the TOTAL/stage console lines when the map:generated event or perf:stage events aren't present, since base's application code may predate that instrumentation (true for master until this PR merges).
The perf.yml job on PR Azgaar#1608 ran all 3 rounds in under 2 minutes (per the log) but then hung until the 60-minute job timeout killed it. spawn()'d the preview server via "npm run preview", which itself spawns vite preview as a further child process; killing the npm process doesn't reliably kill that grandchild, and an unreferenced child handle keeps Node's event loop (and the whole script) alive even after all script logic has finished. Spawn the server detached so it leads its own process group, kill the whole group with process.kill(-pid) instead of child.kill(), and unref() the handle so it can't keep the process alive by itself. Also add explicit process.exit() calls on every path as a hard guarantee. Verified locally: after this fix, ab.mjs exits immediately once reporting is done and leaves no process bound to the preview ports.
The hang fixed in the previous commit burned a full hour of CI time before the job timeout caught it. The actual A/B comparison (build x2 + 3 rounds) takes a couple of minutes locally; 20 minutes leaves generous headroom for a slower CI runner without masking a stuck job for an hour.
CI run on PR Azgaar#1608 flagged markupGrid (+46.5%, spread 51.5%), placePoints (+32.6%, spread 71.4%) and drawRoutes (+26.5%, spread 18.2%) as regressions with 3 rounds. No checksum mismatch, so generation output was identical; these are few-millisecond stages whose relative noise on a shared runner swamps the signal even above the earlier 2ms floor. Only gate on the per-seed `total` and the interaction `gesture` metric, which have stayed stable (single-digit % or better) across every real comparison run so far, local and CI. Per-stage timings are still reported for visibility but can no longer fail the build on their own. Verified against real origin/master: same false positives from the previous run no longer trip the gate, and the run still passes overall.
Owner
|
@SheepFromHeaven , are we done here, ready to merge? |
Owner
|
@SheepFromHeaven , sorry for the conflicts, Pipeline did change the console.time work so the PR should be reworked again. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds Playwright-based performance benchmarks for real map generation and interaction, with an A/B comparison harness that runs base and head alternately in CI. Replaces #1601.
I also reworked the existing
TIME-guardedconsole.time/console.timeEndinstrumentation into atimeStart/timeEndhelper that dispatches aperf:stageevent alongside the console output, and extendedmap:generatedto carry the total time. The generation spec listens to these events instead of parsing console text.New pieces:
tests/perf/generation.spec.ts=> generates real maps for fixed seeds and records per-stage timingstests/perf/interaction.spec.ts=> loads the newesttests/fixtures/*.mapfixture and scripts a zoom/pan gesturetests/perf/ab.mjs=> builds and serves base and head from separate git worktrees, alternates rounds, compares median per-metric ratiostests/perf/playwright.config.ts=> points the perf specs at whichever serverab.mjsis currently serving.github/workflows/perf.yml=> runs the A/B comparison on PRs with read-only permissions.github/workflows/perf-comment.yml=> posts results as a PR comment from a trustedworkflow_runcontext, so it works for fork PRs toosrc/utils/perfEvents.ts=>timeStart/timeEndhelpersNote: since this PR introduces the perf suite,
masterhas nothing to compare against until it merges, and the comment workflow won't activate until it exists onmastereither.