Repository navigation
docs(adr): propose benchmarks observed, not gated, with flag thresholds #630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
8f5487d
docs(adr): propose benchmarks observed, not gated, with flag thresholds
ss-o b4edce1
docs(adr): format the index table
ss-o 692f4a7
docs(adr): register ADR-0024 and state the schema and naming accurately
ss-o 6747567
docs(adr): accept ADR-0024 (decider ss-o)
ss-o 63d5561
docs(adr): correct identity fields, control schema, and references
ss-o File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| # 24. Benchmarks Are Observed, Not Gated, With Flag Thresholds and Committed Per-Release Results | ||
|
ss-o marked this conversation as resolved.
Resolved
|
||
|
|
||
| - **Status:** ACCEPTED | ||
| - **Date:** 2026-09-18 | ||
| - **Deciders:** ss-o | ||
|
ss-o marked this conversation as resolved.
Resolved
|
||
| - **Supersedes:** None | ||
| - **Superseded by:** None | ||
|
|
||
| ## Context | ||
|
|
||
| Two repositories ship benchmarks with different shapes. `z-shell/zpmod` runs a Zsh harness that emits JSON, TSV, SVG, and Markdown, commits per-release results under `benchmarks/results/<tag>-<os>-<arch>/` (today `v2.0.6-linux-x86_64`), and states that hosted-runner results are evidence, not a blocking gate. `z-shell/z-a-meta-plugins` drives a baseline-versus-candidate comparison from Python on every pull request: alternating order, five warmups, thirty samples, median and p95, a functional failure invalidating the timings, and no timing thresholds. `z-shell/zi` is adding a third suite ([z-shell/zi#553](https://github.com/z-shell/zi/issues/553)) after a promotion review found a +20% ice-tokenizing regression only through an ad hoc A/B ([z-shell/zi#554](https://github.com/z-shell/zi/issues/554)). | ||
|
|
||
| Nothing in `decisions/`, `PATTERNS.md`, or the runbooks records how benchmark results relate to merging, which statistics a comparison must report, or where results are published. [ADR-0009](0009-testing-ci-strategy.md) settles the analogous question for coverage ("observed, not gated") but is silent on performance. Each repository has therefore decided alone, and the third suite would have been a third convention. | ||
|
|
||
| ## Decision | ||
|
|
||
| 1. **Observed, not gated.** A benchmark job never fails on timing. It reports, and the maintainer decides. This extends ADR-0009's coverage rule to performance and matches zpmod's stated stance on hosted runners. | ||
| 2. **Flag thresholds for review.** A comparison marks a case for review when its median regresses by more than 10 percent or its p95 by more than 15 percent against the baseline measured in the same run. A flag is a notice on the job and the pull request, not a failure. Repositories may tighten the thresholds, not loosen them, and must record the values in their benchmark README. | ||
| 3. **Comparison shape.** A comparison measures baseline and candidate in the same runner with alternating (balanced) order, discards warmup rounds, reports median, p95, minimum, and sample count per variant and the median and p95 deltas, and includes an A/A control (the baseline measured twice) so the noise floor is visible next to the real comparison. A functional failure on either side invalidates that case and fails the job, because timing a broken behaviour is meaningless. | ||
| 4. **Determinism.** Benchmark workloads are network-free and fixture-driven. Network-dependent behaviour belongs in smoke or integration jobs, not in a comparison. | ||
| 5. **Publication.** Per-release results are committed by a maintainer under `benchmarks/results/<tag>-<os>-<arch>/`, the naming zpmod already uses, after a release, together with the exact source revision, Zsh version, architecture, and runner or host identity. Automation uploads artifacts and job summaries; it never commits results. Results are compared only when the environment identity matches: Zsh version, architecture, and runner or host identity, plus equal sample and warmup counts. Source revisions are expected to differ; they are what a comparison is about. | ||
| 6. **Canonical comparison schema, version 1.** A comparison report carries `schema_version: 1`, `baseline` and `candidate` identity (`label`, `source_revision`, `environment{zsh_version,architecture,cpu,runner_image}`, `workload{warmups,samples}`), a boolean `comparable` derived from those fields, and `cases[case]` where each case is either `{failure: {baseline, candidate}}` or `{results: {baseline, candidate}, change, flag}` with `results[variant] = {median, p95, min, count, samples}` and `change = {median_delta_ms, median_delta_percent, p95_delta_ms, p95_delta_percent}`; `control[case]`, present whenever the A/A control from decision 3 was measured, carries the same row shape with the control measurement in the `candidate` position, so the noise floor is exchangeable and validatable alongside the comparison; `flagged` and `failed` list the affected case names, `failed` covering control rows as well. `z-shell/zi` implements this schema. `z-a-meta-plugins` today emits `results[variant][metric]{median,p95,samples}` and `change[metric]{median_delta_ms,median_delta_percent}` per metric; it adapts to version 1 (adding `min`, `count`, and the p95 deltas, and lifting its per-metric nesting to one case per metric or an explicit `metrics` map in a schema revision) before the shape is admitted to `PATTERNS.md` as an observed two-repository pattern. Until then the pattern is proposed, not observed. | ||
|
|
||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
|
|
||
| - A performance regression becomes visible in the pull request that introduces it, with its size, without blocking unrelated work on hosted-runner noise. | ||
| - Three repositories share one vocabulary, so a reader can compare reports and reviewers know what a flag means. | ||
| - Committed per-release results give every release a performance record tied to an exact revision. | ||
|
|
||
| ### Costs and risks | ||
|
|
||
| - A flag that nobody reads is a regression that ships. The weekly organization review ([`runbooks/org-review.md`](../runbooks/org-review.md)) lists flagged benchmark runs under "Needs review". | ||
| - Hosted runners are noisy; the A/A control makes that visible but does not remove it. Repeated flags on the control column mean the runner, not the change. | ||
| - The thresholds are chosen from the first zi A/B, where unrelated cases sat within ±1 percent and the real regression was +20 percent; they should be revisited after the first ten flagged runs. | ||
|
|
||
| ## Alternatives considered | ||
|
|
||
| 1. **Blocking above a threshold.** Rejected for now: without an A/A history, hosted-runner noise produces false timing failures on unrelated pull requests, and every one of them would have to be waved through by the maintainer bypass that ADR-0013 keeps available for deadlocks, turning an escape route into routine practice. Revisit once the control column has a track record. | ||
| 2. **Report only, no thresholds.** Rejected: z-a-meta-plugins' experience is that unhighlighted tables are not read; the zi regression was found only because a human ran and read an A/B. | ||
| 3. **One central benchmark runner for the organization.** Deferred: zpmod measures a compiled module and z-a-meta-plugins measures real plugin groups; the workloads differ too much for one runner, while the report shape can be shared. | ||
|
|
||
| ## References | ||
|
|
||
| - [z-shell/.github#629](https://github.com/z-shell/.github/issues/629): the decision request. | ||
| - [z-shell/zi#553](https://github.com/z-shell/zi/issues/553): the zi suite; [z-shell/zi#554](https://github.com/z-shell/zi/issues/554): the measured ice-tokenizing regression; [z-shell/zi#555](https://github.com/z-shell/zi/issues/555): the manifest-reader cost recorded as a baseline and optimization target. | ||
| - `z-shell/zpmod` `benchmarks/README.md` and `z-shell/z-a-meta-plugins` `benchmarks/README.md`: the two existing conventions. | ||
| - [ADR-0009](0009-testing-ci-strategy.md): coverage observed, not gated. | ||
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.