perf(bench-ratchet): scope the -count reducer — min for the informational A/B, mean for the ratchet

[mparrett]

Reworked after @nooga's review. The original version reduced -count repetitions by min everywhere; the compounding-with-the-ratchet catch was right, so this scopes the reducer — mean by default, min only where there's no persistence — and addresses all three review points (mapping at the bottom).

What this does

aggregateFromFile collapses the N -count repetitions of each benchmark into one number. That reducer is now caller-selected via a -reducer flag:

• mean (default) — for anything feeding the persistent docs/perf/baseline.json ratchet. ratchetMerge already takes the min over all history, so feeding it a min-of-N would stack two minimums: one lucky-fast sample, at any point in history, permanently sets a floor legitimate future runs may never clear. Mean keeps the durable bar honest.
• min (opt-in) — for the throwaway per-run informational A/B (perf-pr.yml). Benchmark noise there is one-directional (GC pauses, CPU migration, a co-scheduled tenant, thermal throttling only ever make a run slower), so the fastest of N is the least-contaminated estimate. No persistence, so nothing compounds.

The reducer and sample count are recorded on the baseline (schema v2), so a min-era and a mean-era snapshot are never silently diffed — a provenance mismatch re-seeds with an explicit "intentional discontinuity" note instead of ratcheting a min bar onto a mean one.

perf-pr.yml passes -reducer min on both A/B halves. The base half builds bench-ratchet from the merge-base commit, which can predate the flag, so it feature-detects -reducer and falls back to the default when it's unsupported — keeping both halves on the same reducer rather than crashing on an old base. (Validated on a fork self-A/B.)

docs/perf/baseline.json was captured at -count 1, where min == mean, so it's migrated to v2 by a metadata-only stamp — the numbers are untouched, no re-benchmark on a foreign CPU.

Scope is cmd/bench-ratchet + the perf-pr.yml invocation; still informational-only, never a gate. The residual noise on larger memory-bound families (whole capture-windows slowed at once, where min can't help) is the separate repeat-the-A/B gate I'm coordinating on elsewhere.

Field evidence — real perf-pr A/B, min vs mean

On the same-runner A/B this reducer feeds, the same VM-optimization stack produced opposite verdicts depending on which reducer was in play:

perf-pr run	reducer (both sides)	BenchmarkGetOwn/n=64/last	verdict
fork #12	mean	+20.1%	1 regression > 10% budget
fork #15	min	−2.4%	0 regressions > budget

GetOwn touches nothing those PRs changed — the +20.1% was pure runner noise the mean amplified into a phantom regression; under min it collapses to −2.4%, and the whole family lands within ±5.4%. (Caveat: #12 and #15 are separate PRs on different runners, so the reducer isn't the only variable — read this as corroborating field data. The controlled proof is below.)

Local stability, GetOwn/n=16/last, count=6 across 3 captures: mean-of-6 spread 3.1% vs min-of-6 0.8% (~4× tighter; wider on noisier CI).

Controlled proof (go test, no CI needed)

• TestAggregateFromFileUsesMin — pins that -count repetitions reduce by min under -reducer min, with raw samples retained.
• TestReducerChoiceChangesRegressionVerdict — feeds 15 real captured GetOwn/n=16/last samples (a clean cluster + a contention-slowed upward tail) through the actual compareAndReport at the 10% budget. Same samples, opposite verdict: min → 0 regressions (true cost), mean → 1 regression (+16% phantom).
• TestMinReducerRobustAtProductionCount3 — at the shipped -count 3, min-of-3 is invariant to a single upward-contaminated sample (stays pinned to the clean floor) while mean-of-3 moves. Confirms min is stable, not outlier-chasing, at the production count.

Responding to the review

1. Reducer compounding with ratchetMerge → scoped: mean is the default (persistent ratchet), min is opt-in (informational A/B only). aggregateFromFile is no longer unconditional min.
2. Evidence at N=3, not just N=15 → added TestMinReducerRobustAtProductionCount3 at the shipped count.
3. baseline.json not migrated → stamped to v2 (reducer/sample_count); faithful metadata-only migration since it was captured at -count 1.

go build/vet/gofmt clean; actionlint clean; cmd/bench-ratchet tests pass. Composes with the other open perf PRs — the only conflict is a documented 3-line one with #24 in aggregateFromFile (see comment below).

[mparrett]

Reviewed. Conditionally defensible as a low-sample, best-case heuristic: but not with the PR’s current statistical claims. I’d request two changes before merging.

• [P1] The reducer and sample count are absent from the persisted schema. A minimum is count-dependent: increasing -count increases the opportunity to observe a lower value. Mean-era and min-era snapshots therefore aren’t directly comparable, yet both remain schema version 1. Record reducer and sample_count, reject incompatible baseline comparisons, and annotate or split the timeline at this transition. See main.go:641 and perfdata.go:5.

• [P2] The “controlled proof” demonstrates verdict suppression, not that the suppressed result is phantom noise. Of its 15 GetOwn samples, 10 are in the slower ≥12 ns regime. The ratios are approximately: minimum 8.14, mean 9.46, median 10.32. Because the test defines the accepted baseline as the minimum, its conclusion is largely built into the setup. The premise that noise “only ever makes a run slower” is also too absolute; frequency scaling, cache state and ordinary measurement variation can produce favorable samples. Reframe this as selecting the lower performance envelope under assumed upward contamination, not estimating “true cost.”

For the current informational, same-runner A/B report, min-of-3 is an acceptable pragmatic heuristic if those limitations are explicit. It should not become a performance gate. The Go team’s benchmark tooling instead recommends at least 10 samples and reports medians, confidence intervals, and distribution comparisons. [Official benchstat documentation](https://pkg.go.dev/golang.org/x/perf/cmd/benchstat).

My recommended path:

1. Keep min temporarily for the informational PR report.
2. Add reducer/count provenance and mark the timeline discontinuity.
3. Soften the PR description and test names—“illustrates lower-envelope behavior,” not “proves phantom regression.”
4. Make the planned repeated A/B—or benchstat with ≥10 samples—the eventual defensible gate.

Implementation tests pass locally (go test ./cmd/bench-ratchet), and the PR’s CI/lint checks are green. The concern is interpretation and data compatibility, not code correctness.

[mparrett]

LLVM guidance may be helpful here: It mostly argues for improving experiment design, not choosing min.

LLVM guidance	Paserati status	Recommendation
Run repeatedly to recognize noise	Partial: -count 3, but aggregation collapses samples to one minimum	Preserve and compare distributions; ideally ≥10 samples with benchstat
Eliminate measurement bias	Weak: PR workflow always runs all base measurements, then all head measurements	Interleave runs—ABBA or randomized blocks—after prebuilding both test binaries
Disable frequency scaling, Turbo, competing work; isolate CPUs/SMT	Not available on shared ubuntu-latest runners	Keep results informational, or use a controlled self-hosted runner for gating
Avoid storage	Microbenchmarks mostly comply; Test262 reads thousands of files and writes per-shard JSON	Put Test262 corpus/output on tmpfs or prewarm it when measuring execution time
Use a high-resolution timer / perf	Go’s benchmark timer plus large b.N is adequate for timing; hardware counters are absent	Longer benchmark windows help tiny operations; perf stat would be useful diagnostically on a dedicated runner
Prefer static linking	Largely satisfied by this pure-Go/no-CGO project	No major action
Disable ASLR	Potentially relevant but lower priority than CPU isolation and ordering	Only worthwhile on a dedicated runner

The most important issue is fixed ordering in perf-pr.yml:91. The sample set used in PR #22 actually looks like a regime transition—five fast measurements followed by ten slow ones. If that transition happens between the base and head phases, neither mean nor minimum can tell environmental drift from a code change. Interleaving is the direct remedy.

LLVM explicitly warns that low noise is insufficient because measurement bias can remain, and recommends repeated measurements plus system control. LLVM benchmarking guidance.

Two additional conclusions for our setup:

• For same-runner PR comparisons, anchor normalization is probably unnecessary and may add noise because the arithmetic anchor and memory-sensitive benchmarks react differently to system conditions. Compare raw base/head distributions.
• For the cross-runner timeline, keep the anchor but only compare matching CPU model, Go version, architecture, reducer and sample count. Anchor normalization cannot make heterogeneous runners genuinely equivalent.

So I’d prioritize:

• Prebuild base and head benchmark binaries.
• Run them in ABBA or randomized order.
• Retain ≥10 samples and use median/confidence intervals through Go’s official benchstat, which explicitly recommends at least ten runs. benchstat documentation.
• Keep shared-runner results informational.
• Use a controlled runner before introducing a gate.

The LLVM document does not really support “minimum estimates true cost.” Its recommendation to run repeatedly “to recognize noise” supports retaining and analyzing the sample distribution rather than reducing it immediately in main.go:641

[mparrett]

Both comments make sense- the fixes are in (61d8e49).

Provenance / compatibility (your P1). Done. perfdata.Baseline now records reducer + sample_count, and schemaVersion goes 1→2. A mean-era v1 baseline is rejected on read, so min-era and mean-era snapshots can't be silently diffed; update re-seeds on the mismatch with an explicit "intentional timeline discontinuity" note rather than ratcheting a min bar onto a mean one. compareAndReport also warns on a same-version reducer/count mismatch — a forward guard for the median reducer below.

Reframe (your P2). Agreed the "true cost / phantom" framing overclaimed. The sample set is a regime transition (median 12.37, well above the min), so the minimum is a lower envelope, not an estimate of truth. Reworded the aggregateFromFile rationale and renamed the test to TestReducerChoiceChangesRegressionVerdict; it now states plainly that it shows verdict suppression under an upward-contamination assumption — informational-only, never a gate.

On ordering, and the defensible gate (second comment). You're right that fixed base-then-head ordering is the deeper issue and that min-of-N must not gate. The gate you describe — repeated, interleaved A/B with a distribution comparison — is already built: an N=7 interleaved A/B with ABBA counterbalance (odd cycles base-first, even head-first), 10% budget, 0 false positives across 10 no-op runs. It composes with this reducer as min within a capture / median across cycles, and it'll come as a PR stacked on this one. Your regime-transition observation is a clean argument for exactly that interleaving — I've folded it in.

So this PR stays the minimal informational reducer + provenance; the interleaved gate is the follow-up.

[nooga]

Overview

Good, well-documented change with real test coverage and unusually honest comments about the reducer's limits. The core motivation — min is less contaminated by one-directional noise than mean — is standard benchmarking wisdom, and the included verdict-flip test is a genuinely convincing demonstration for the informational perf-pr comparison.

Concerns

1. Compounding with ratchetMerge: ratchetMerge, used by update/check against the persistent docs/perf/baseline.json, already does minF(cur, base) across commits forever — the bar never loosens without -force. This PR makes the input to that process itself a min-of-N. Two stacked minimums mean one lucky low sample, at any point in history, permanently sets a floor that legitimate future runs may never clear. The PR description scopes this as "informational-only," but aggregateFromFile is unconditional shared code feeding both paths. Please either scope the reducer choice to the informational path only, or add a test/analysis showing the long-term ratchet path is still sound.

2. Evidence uses N=15, production uses N=3: perf-pr.yml/perf-timeline.yml both run -count 3. The controlled proof test uses 15 samples. Worth adding a variant at N=3 (the actual shipped configuration) to confirm min-of-3 is stable and not just a different flavor of outlier-chasing on these sub-20ns benchmarks.

3. docs/perf/baseline.json not migrated: it's still version: 1. Post-merge, bench-ratchet check against the default path will hard-fail until someone runs update once. Please regenerate it as part of this PR (or explicitly call out the required manual step so it isn't forgotten).

Nits

• gofmt/go vet clean, build passes, new tests pass as-is.
• No test covers ratchetMerge interacting with min-reduced input — worth one given concern #1.

[mparrett]

Addressed in ad52ea0 — all three concerns:

1. Reducer compounding (P1). Added a -reducer flag: mean is now the default (safe for the durable ratchet, since ratchetMerge already takes min-over-history — min-of-N input would compound into an unclearable floor), and 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 min-era and mean-era points are never silently diffed. aggregateFromFile is no longer unconditional min.

2. N=3 evidence. Added TestMinReducerRobustAtProductionCount3: at the shipped -count 3, min-of-3 is invariant to a single upward-contaminated sample (stays pinned to the clean floor) while mean-of-3 moves — demonstrating min is stable, not outlier-chasing, at the production count.

3. baseline.json migration. The committed docs/perf/baseline.json was captured at -count 1 (one sample/benchmark, where min == mean), so I did a faithful metadata-only stamp — version 2, reducer "mean", sample_count 1 — numbers untouched (no re-benchmark on a foreign CPU). The version gate now passes; check won't hard-fail post-merge.

build/vet/gofmt clean; bench-ratchet tests pass; actionlint clean.

[mparrett]

Heads-up: this composes with #24 and there's one small merge conflict. Both this PR and #24 edit cmd/bench-ratchet/main.go's aggregateFromFile. Verified by composing all open PRs on a clean nooga/main base — this is the only conflict across the set, and it's a 3-line resolution in the Result{...} build: keep this PR's reducer form and add #24's SetHash:

NSPerOp:     reduce(a.nsMin, a.nsSum, a.count),
BytesPerOp:  int64(reduce(a.bytesMin, a.bytesSum, a.count)),
AllocsPerOp: int64(reduce(a.allocMin, a.allocSum, a.count)),
SetHash:     a.setHash,

The accumulator auto-merges (each PR adds distinct fields); only this block collides. Either order is fine — whichever lands second applies the above.

[nooga]

Thanks Matt — the reducer scoping (mean for the ratchet, min for informational A/B) and provenance stamping look correct and well-evidenced. Approving, but this now conflicts with main since #24 (fingerprint) landed first and touches the same aggregateFromFile — as you'd already flagged. Could you rebase onto main and I'll merge right after? No changes needed beyond the rebase.

[mparrett]

@nooga Thanks for taking the time to look at this! Rebased as requested (2 conflicts resolved). Looks green, so feel free to merge unless there are remaining concerns.

[mparrett]

Summary of what changed:

• Two conflicts, both in cmd/bench-ratchet. main.go's Result{...} build was the 3-line one you'd already documented. main_test.go was an add/add — bench-test262: fingerprint the contributing test set so test262.total is comparable #24 and this PR each created the file with different tests; kept both sets.
• One semantic conflict git couldn't detect — bench-test262: fingerprint the contributing test set so test262.total is comparable #24's aggregate helper predated the -reducer parameter. Fixed inside the commit that changed the signature, so every commit in the stack still builds.
• Verified at each of the 5 commits via git rebase --exec, not just the tip.
• pkg/perfdata/perfdata.go is gofmt -l-dirty on upstream/main already (struct-tag
  alignment from bench-test262: fingerprint the contributing test set so test262.total is comparable #24). Left it alone rather than mixing a format fix into this PR. Local perf/bench-ratchet-min now points at 4fdad50; local perf/vm-micro-opts working tree was never touched.