feat(perf): add local performance regression checks - #259
Conversation
doodlewind
left a comment
There was a problem hiding this comment.
Thanks for the substantial work here. The versioned protocol, exact QEMU counters, marker checks, and receipts are a useful direction, but I am requesting changes because the current implementation can report pass for executions whose workload or provenance is not actually comparable.
Blocking issues:
- The GitHub workflow is A/A, not base-vs-head: Native uses
--base HEAD, and both QEMU runs use the same checkout. A fixed regression in the PR affects both sides and still passes. Please keep A/A as a separately named determinism check and add a real trusted-base comparison if this workflow is intended to gate regressions. - The Vapor scenario declares
vapor/examples/todo/todo.tsx, while the executor compiles and source-patchestodo.playdate.tsx; the Native artifact hash still hashes the declared, unexecuted file. Inperf local, onlytools/perf/appsis frozen, so base and candidate may also compile different production Vapor examples. The manifest, materialized source, executed artifact, and receipt hash need one identity. PerfRunSummaryV1does not bind a run to one source revision/content hash or toolchain/executor fingerprint. A directory containing valid receipts from different revisions can currently compare aspass. Please bind the run manifest to immutable source/harness/toolchain identities and verify every listed receipt against them.- Declared correctness checkpoints are not enforced as an exact set. A Native result with no checkpoints can still produce a valid receipt for a scenario that declares them, and some paths compare only final state. Missing or extra checkpoint captures must invalidate the run, and the measured replay needs phase-boundary correctness evidence.
- QEMU execution records an image ID once but continues launching containers by the shared mutable
pocketjs-perf-qemu:11.0.3tag. A concurrent build in another worktree can retag the image during a suite while receipts still claim the original ID. After inspection, all version checks, builds, runs, and cleanup should use the immutable image ID; build tags should also be content- or worktree-scoped.
Before re-review, please also reduce the drift surface: replace Vapor source-string surgery with a committed harness-owned fixture and explicit Linux host, consolidate the multiple canonical JSON/hash implementations and Docker runners, and keep repo-only perf assets out of the published npm tarball unless shipping them is intentional and tested.
The current green checks are useful evidence that the candidate harness builds and repeats under A/A; they do not resolve the comparability and provenance failures above.
Summary
Validation
bun test tests/perf-*.test.ts— 77 passGIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=commit.gpgsign GIT_CONFIG_VALUE_0=false bun tools/test.ts --stage=unit— 654 passbunx tsc --noEmitbun perf doctor --json— Native and both QEMU executors readyScope