[ci-test] #22 reducer scoping — validate perf-pr on fork - #17
Closed
mparrett wants to merge 5 commits into
Closed
Conversation
aggregateFromFile averaged the N -count repetitions of each benchmark. Mean is the wrong reducer for benchmark noise, which is one-directional: interference (GC pauses, CPU migration, a co-scheduled runner tenant, thermal throttling) only ever makes a run slower, never faster. A single slow sample drags the mean up and manufactures a phantom regression — exactly what the small (<20ns) BenchmarkGetOwn variants did on shared CI runners, flip-flopping 6/2/4 "regressions" across three otherwise-identical perf-pr runs on code paths the PRs never touched. Reduce by minimum instead: the fastest of N repetitions is the least contaminated estimate of true cost. All raw samples are still retained in Samples for provenance. alloc/bytes are deterministic per op (min == mean). Local stability check, BenchmarkGetOwn/n=16/last, count=6 x3 captures: mean-of-6 spread across captures: 3.1% min-of-6 spread across captures: 0.8% (~4x tighter; larger on noisier CI) Adds cmd/bench-ratchet/main_test.go (first test for the package) pinning the min reduction. Note: perf-pr's base-vs-head A/B benefits immediately and safely (both halves measured with the same reducer in one run). perf-timeline snapshots on the perf-data branch were captured with mean, so the first snapshot after this lands steps down once (min < mean) — a one-commit discontinuity on the historical dashboard, not a regression. Do NOT put the `perf` label on this PR: it would compare base's mean-reducer against head's min-reducer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n real samples TestMinVsMeanFlipsRegressionVerdict feeds 15 real captured BenchmarkGetOwn/n=16/last samples (memory-bound; a clean cluster plus a contention-slowed upward tail) and the anchor through the actual aggregate + compareAndReport path at perf-pr's 10% budget: min-reduced ratio 9.536/1.172 = 8.14 -> 0 regressions (true cost) mean-reduced ratio 11.759/1.243 = 9.46 -> 1 regression (+16% phantom) Same samples, opposite verdict — the phantom the min reducer removes, shown deterministically and runnable with `go test`. Complements the existing reduction unit test with the verdict-level effect. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… lower-envelope heuristic Addresses review feedback on nooga#22 — interpretation and data compatibility, not code correctness (implementation tests + CI were green). Provenance / compatibility: - perfdata.Baseline gains Reducer + SampleCount; buildCurrentBaseline stamps them ("min", N observed samples). A minimum is sample-count dependent, so this is the metadata needed to keep min-era and mean-era snapshots from being silently compared. - schemaVersion 1 -> 2: mean-era (v1, no provenance) baselines are now rejected on read. `update` re-seeds on the mismatch and prints an explicit "intentional timeline discontinuity" note rather than ratcheting a min-era bar onto a mean-era one. - compareAndReport warns on a same-version reducer/count mismatch (forward guard for the median-of-N reducer). Reframe (min selects a lower envelope, it does not estimate "true cost"): - aggregateFromFile doc reworded: pragmatic lower-envelope heuristic under an upward-contamination assumption, informational-only, never a gate; notes favorable samples exist and that a defensible gate is the median-of-N repeat A/B, not this reducer. - Rename TestMinVsMeanFlipsRegressionVerdict -> TestReducerChoiceChangesRegressionVerdict. Its 15 real samples are a regime transition (median 12.37), so it demonstrates verdict SUPPRESSION under upward contamination, not proof-of-phantom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…chet Addresses the CHANGES_REQUESTED review's three concerns: 1. Reducer compounding (P1). aggregateFromFile was unconditionally min, but it feeds both the throwaway per-run A/B *and* the persistent ratchet — and ratchetMerge already takes min over all history, so min-of-N input compounds into a floor a single lucky sample can set unclearably low. Add a -reducer flag: "mean" is the default (safe for the durable baseline), "min" is opt-in for the informational path. perf-pr.yml passes -reducer min on both A/B halves (base update + head check, against a RUNNER_TEMP baseline — no persistence). The reducer is recorded on the Baseline, so a min-era and mean-era point are never silently diffed. 2. N=3 evidence. The verdict-flip proof uses N=15; production ships -count 3. Add TestMinReducerRobustAtProductionCount3: at the shipped count, min-of-3 is invariant to a single upward-contaminated sample (stays pinned to the clean floor) while mean-of-3 moves — min is stable, not outlier-chasing. 3. baseline.json migration. The committed docs/perf/baseline.json was version 1, so a post-merge `check` would hard-fail on the version gate. It was captured at -count 1 (one sample per benchmark), where min == mean, so this is a faithful metadata-only stamp — version 2, reducer "mean", sample_count 1 — with the numbers untouched (no re-benchmark on a foreign CPU). go build/vet/gofmt clean; bench-ratchet tests pass; actionlint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rge-bases
The A/B builds bench-ratchet from the merge-base commit, which can predate the
-reducer flag; passing it unconditionally crashes that half ("flag provided but
not defined: -reducer") for any PR branched before this change. Detect support on
the base binary and reuse the decision for the head half via GITHUB_ENV, so both
halves run the SAME reducer: min when supported, and a graceful fall-back to the
default (mean) when the base is too old — like-for-like either way, since this is
the informational, non-gating report. Caught by a fork self-A/B dispatch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Perf — base vs head, same runnerBase |
Owner
Author
|
Done — perf-pr A/B validated green on the fork with the feature-detect fix (nooga#22). Throwaway vehicle, nothing to merge. |
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
1 participant
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.
Throwaway fork-internal PR to exercise
perf-pr.ymlwith the new-reducer minflag on real CI. Not for merge — mirrors nooga#22. Closeable once the perf-pr A/B run goes green.