Add calibrated performance comparison statistics - #356
Open
DanRod1999 wants to merge 15 commits into
Open
Conversation
The quick-performance reporter only described a single run in isolation -- median, CV, MAD, a variance status. None of that compares anything, so it answers "was this run noisy?" rather than "did performance change?". This adds the statistical core for both A/B (two arms in one run) and baseline (this run vs. a stored one) comparison, which share one estimator, one uncertainty model and one report shape. The unit of independence is one pair: one A and one B observation measured adjacent in time with alternating order. Most measured variance is machine-level and common-mode across arms within a pair, so analysing the within-pair difference cancels it. Ratios are analysed on the log scale because performance noise is multiplicative, which also lets results be stated as percentages independent of scenario scale. Verdicts are only issued against an empirically measured A/A noise band for the environment; there is no normal-theory confidence claim at n=8. Notable design points, each of which is a bug avoided: - Sign gates are expressed in exact binomial counts, never transcribed decimals. The exact unanimous level at P=8 is 0.0078125, so a threshold written as `<= 0.0078` makes `regressed` unreachable at any effect size. A dedicated test pins reachability. - The magnitude gate tests the null of the median, not of individual pairs. Those live on different scales, and using the per-pair quantile makes the gate roughly twice as loose for the same executions. - Because that null depends on the pair count, bands cannot be stored as a single number; the pool keeps raw observations and derives per-P. - Arm loading rejects a mismatched core-backend by realpath rather than recording the version and continuing. Two copies of one version are still two module instances, and would dual-load the native addon and the IModelHost singleton. - `unchanged` requires a declared equivalence margin, not the noise floor. Until one is declared it is unreachable and the verdict is `inconclusive`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The noise floor is a property of the measured region, not only of the machine. `incremental-synchronization` rebuilds its fixture per sample on a live-hub topology; that rebuild is outside the timed region, so it never enters `wallMilliseconds`, but it lengthens the process, widens the gap between the A and B measurements of a pair, and leaves different page-cache and GC state going into the measured call. It is symmetric across arms, so it does not bias the estimate -- it raises the floor. A band measured there therefore describes a different noise floor from one measured on an artifact-backed scenario. Transporting it manufactures verdicts in one direction and suppresses real ones in the other. Bands now carry the same key as baselines -- scenario, recipe hash and environment class -- plus `k` and band kind, which describe the process structure the pool was collected under. `noiseBandKey` is the single source of that key so persistence and lookup cannot drift from what `assertPoolApplies` enforces. A lookup miss stays `uncalibrated`; there is no fall-back across scenarios any more than across machines. This makes calibration per-scenario rather than per-machine, at roughly 192 executions each. That cost is real and is documented as a scoping decision -- which scenarios need a verdict at all -- rather than something to be absorbed by sharing a band. Also records why independent per-arm fixture rebuild remains valid for that scenario: generation is deterministic and `semanticDigest` excludes ElementIds, briefcase ids and GUIDs, so the cross-arm digest gate carries the guarantee in place of artifact identity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Everything the specification claims about false-positive rate and detection was prose. Prose cannot notice when a constant stops matching the rule it describes -- which is exactly how A1 survived review: a transcribed decimal threshold that excluded the exact unanimous level and made `regressed` unreachable at every effect size. These tests run the real verdict rule against pairs drawn at known effects and assert what comes out. The A1 defect now shows up as detection collapsing to zero rather than as a number nobody recomputes. Measured at P = 8, effects in per-pair sd, 4,000 trials: effect magnitude consistency verdict 0 (A/A) 0.048 0.009 0.002 0.81 (band) 0.509 0.160 0.127 1.0 0.687 0.260 0.230 1.5 0.953 0.575 0.566 2.0 0.997 0.837 0.836 The consistency gate is binding across the whole usable range -- the verdict column tracks it, not magnitude -- which is what makes escalation worth its budget, since relaxing unanimity to 14/16 is precisely what it does. Also pins one trap. Magnitude power evaluated at the band is ~50% at every pair count, and that is an identity, not a finding: the band is the 95th percentile of the null median and the median is centred on the true effect, so `mu = band` is a coin flip by construction. Read across P it appears to show escalation achieving nothing, when it only shows each P being evaluated at a different effect size, because the band tightens with P. At a fixed effect escalation roughly doubles detection. A test asserts the ~50% identity so nobody re-derives the wrong conclusion from it, and the escalation test holds the effect fixed. Corrects the justification on `escalationRecommended`, which had recorded that misreading, and records the measured table in the specification. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…rgin Arms are now TestTransformerModule-shaped -- the contract the weekly regression suite already dynamic-imports through EXTRA_TRANSFORMERS -- rather than the bespoke "export IModelTransformer" shape this had before. The three arms that already exist become usable here unmodified, and an arm written for either suite works in both. That interop is worth more than a cleaner purpose-built interface. Two optional additions, so every existing module stays valid: - `createChangeProcessingTransform`. The shared contract covers identity and fork-init only; the quick scenario measures `process()` under `argsForProcessChanges`. - `dispose` on the runner. Not tidiness. The existing implementations tear down inside `run()` -- NativeTransformer calls `transformer.dispose()` and `editTxn.end()` there -- so the arm rather than the harness decides what falls inside the timed region. Harmless for a weekly number, unacceptable when resolving a few percent: two arms could fold different work into `run()` and the difference would be indistinguishable from a real effect. A/A calibration cannot catch it either, since identical code in both arms produces no cross-arm boundary difference. Arms supplying `dispose` get teardown outside the measured region; arms that do not are still accepted and the report records it. Also confirms subprocess co-resolution empirically, which was the open risk that could have invalidated baseline persistence. Two arm packages carrying different transformer versions, peers linked to the harness realpaths, loaded in separate processes: both resolved one byte-identical core-backend realpath. pnpm does not hoist core-backend, so bare resolution fails even inside the workspace; only peers may be redirected, since the arm's own `semver` is part of what is under test. Subprocess isolation is retained for measurement independence, not to allow arms to differ in core-backend -- 6.1 forbids that. In-process arms would have the second arm measured on a heap the first had warmed, and order alternation cancels the mean of that while inflating the per-pair spread the design exists to reduce. Declares the equivalence margin at 5%, per Nam. It is a relevance threshold on a between-arm difference and is deliberately not derived from the noise floor, so `unchanged` stops being hostage to how quiet the machine was. It coincides numerically with the classifyVariance constant by intention; the two are different quantities and nothing couples them. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…not be An equivalence margin below the noise floor blocks `unchanged`, but change verdicts are decided against the band and never against the margin. That branch was returning `escalationRecommended: false` unconditionally, so a run whose magnitude gate had already cleared the band -- and which needed only the relaxed 14/16 unanimity of look 2 to report a regression -- terminated at look 1 instead. Escalating helps twice here: unanimity relaxes from 8/8 to 14/16, and the band itself tightens at the larger pair count, which can lift the floor back under the margin and make `unchanged` reachable again. This branch is live precisely where it does the most damage. Local macOS runs measured CV 2-6% against a declared 5% margin, so the noisiest environments -- the ones most likely to need the extra pairs -- were the ones being denied them. Also records the expected execution budget, measured through the shipped rule rather than assumed. The conditional trigger fires far less often than `inconclusive` occurs: at zero true effect, 99.8% of runs are inconclusive but only 4.6% escalate, so a quiet week costs ~67 executions rather than the 128 worst case. Escalating on undifferentiated `inconclusive` would have made 128 the routine cost, which is the concrete payoff of narrowing the trigger. Derives the margin resolvability condition while there: with band = c(P)*sigma and margin = ln(1.05), the declared 5% is resolvable only if the per-pair spread is under 6.03% at P = 8, or 8.31% at P = 16. That is a falsifiable acceptance criterion for the first A/A run rather than something to discover afterwards. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The gate was challenged as redundant: A2 made the band an empirical resample of the real A/A pool, so the magnitude gate is already nonparametric and the consistency gate's distribution-free justification looked spent. Under unanimity it was costing 1.7x in detectable effect (80% power at 2.38x band vs 1.42x unprotected) to hold false positives at 0.24% against a 5% budget -- 20x more protection than anyone asked for. Measured rather than assumed. The magnitude gate is nonparametric in the SHAPE of the per-pair distribution but not in its SCALE, and bands are persisted and reused, so drift between calibration and comparison is the expected operating condition rather than an edge case. False positives at zero true effect, band calibrated at one spread, run drawn at a multiple (20,000 trials per cell): run spread magnitude only +8/8 +7/8 +6/8 1.0x 4.88% 0.24% 1.48% 3.68% 1.5x 18.75% 0.66% 3.96% 11.67% 2.0x 32.00% 0.74% 5.44% 17.70% 3.0x 50.97% 0.83% 6.53% 23.96% Local runs already span 2.17-6.43% CV on one machine class, so the 3.0x row is roughly the observed range. The sign test's null is 50/50 under any zero-median distribution whatever the spread, which is what holds its columns flat while a magnitude-only rule degrades to a coin flip. The alternative -- detect staleness from the run's own spread -- was measured and cannot substitute: at 1.5x drift it flags 34% of runs while false-flagging 6.7% of well-calibrated ones. So the gate stays, at the loosest count that keeps the combined rate near budget across the drift range. Adopting 7/8 moves the operative 80% power point from 2.38x band to 1.67x while bounding false positives at 6.5% even when the stored band badly understates current noise. Implemented as one constant: signGateTargetLevel 0.01 -> 0.10, since signGateRequirement already derives counts from a declared level. The level is declared on the CONJUNCTION -- the rule that ships -- so the marginal per-gate level is a diagnostic and never the operative rate. Spec updated in the same change: new S4.5 records the measurement, S4.3 carries the new counts, S4.4 reports power for the combined rule only, and S1.3/S3.1 carry re-measured escalation rates (4.0% at mu=0, expected 67 executions). Look-2 false positives measured at 2.09% with the band re-derived at P=16, as the rule requires. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The margin is an ACTION threshold: `unchanged` asserts "we looked, and any real change is below what we would act on". A transformer regression small enough to sit under 10% is not one anyone opens work for, so 10% is the honest statement of that claim. The choice materially moves the acceptance criterion for the first A/A run, which is the part worth recording. With band = c(P) * sigma_d: P c(P) margin resolvable iff (at the former 5% margin) 8 0.819 sigma_d <= 11.63% 5.96% 16 0.614 sigma_d <= 15.53% 7.95% 24 0.499 sigma_d <= 19.12% 9.79% At 5% the criterion sat INSIDE the range of single-run CVs already observed locally (2.17-6.43%), so `unchanged` was plausibly unreachable on the machines this suite actually runs on -- before pairing was even credited with cancelling common-mode noise. At 10% the bar is comfortably above the worst observed CV, so the A/A run is expected to clear it rather than expected to fail it. That is not licence to assume the result: if A/A returns sigma_d near or above 11.63%, the response is more pairs or a longer measured region, never a wider margin. Pinned by a new test rather than prose. The criterion is the product of two independently movable things -- the declared margin and the band coefficient -- so a stated constant goes stale silently the moment either is tuned. The test derives it from the shipped constants and asserts the boundary behaves like one: just inside it the verdict is `unchanged`, just outside it `inconclusive` with the unresolvable-margin reason. Also corrects a claim in the earlier draft. The spec had cited this margin's former numerical agreement with the CV constant in BenchmarkReporter as making the eventual retirement of that constant a cleaner story. That was wrong and is now removed: a CV threshold on within-run dispersion and a relevance threshold on a between-arm median are different quantities on different scales. They shared a numeral, not a derivation, and the two retirements are independent. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The 80%-power point is a property of the CONJUNCTION of the magnitude and consistency gates, so it moves whenever the consistency requirement moves. That coupling is invisible at every call site, and it has already caused a wrong number to circulate: 1.42x band is the magnitude-gate-alone figure and is correct only if the consistency gate is removed entirely. Under the shipped AND rule the operative point is 1.67x, and quoting the former understates the detectable effect by ~18%. Documented as a table at signGateTargetLevel, which is the single constant that moves all of it, alongside the false-positive columns that explain why the gate is a gate rather than a diagnostic. The 1.67x value is already pinned by test, so a silent revert fails loudly rather than quietly restoring a stale power claim to every report. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Separate statistical analysis from runner execution policy, enforce complete calibration identity, and retain magnitude-led paired verdicts with focused diagnostics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 29, 2026
Keep comparison statistics coverage on the performance package's Vitest-only quick-unit conventions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
DanRod1999
added a commit
that referenced
this pull request
Jul 29, 2026
## Summary - migrate the weekly performance regression suite from Mocha to serialized Vitest execution - remove performance-tests ownership of Mocha, `@types/mocha`, Chai 4, and `ts-node` - preserve the weekly report and authentication contracts ### New test helpers These helpers are not required by Vitest. They make behavior that was embedded in the previous Mocha suite explicit and independently testable: - `Cleanup.ts` guarantees that every registered teardown task is attempted, including database closure, report export, `IModelHost` shutdown, and authorization sign-out. If the operation and cleanup both fail, it preserves both errors instead of allowing teardown to hide the primary failure. - `RegressionTestRegistration.ts` constructs the supported test-case/transformer-module combinations synchronously before Vitest registers tests. It skips modules that do not implement the operation required by a test case, preserving the old conditional Mocha registration behavior without asynchronous collection callbacks. - `Cleanup.test.ts` and `RegressionTestRegistration.test.ts` provide focused, non-credentialed coverage for those behaviors. This PR targets `main` and is intended as the new bottom layer beneath the existing quick-performance PRs #353, #354, #356, and #357. It does not modify those branches or their stack metadata. ## Validation - `pnpm --dir packages/transformer build` - `pnpm --dir packages/performance-tests build` - `pnpm --dir packages/performance-tests lint` - `CI=true pnpm --dir packages/performance-tests exec vitest run test/unit/Cleanup.test.ts test/unit/RegressionTestRegistration.test.ts` (4 tests passed) - verified the performance-tests manifest and lockfile importer no longer declare Mocha, `@types/mocha`, Chai 4, or `ts-node` The credentialed Hub-backed weekly runtime suite was intentionally not run locally. Hosted weekly Azure validation remains deferred, consistent with the source migration handoff. *generated with GPT-5.6 Sol* --------- Co-authored-by: Nam Le <50554904+hl662@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Daniel Rodriguez <DanRod1999@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Place comparison sources, units, and specification alongside the reorganized quick performance harness without changing calibrated behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
2 participants
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.
Summary
Stack
Layer 3 above #354 and #353. This PR targets
danrod1999-changeset-scanning-benchmark; it does not add or mutate native stack metadata.Validation
pnpm --dir packages/performance-tests buildpnpm --dir packages/performance-tests exec eslint "test/quick/comparison/*.ts"generated with GPT-5.6 Sol