diff --git a/.github/bench/README.md b/.github/bench/README.md index a9e4d742..06218717 100644 --- a/.github/bench/README.md +++ b/.github/bench/README.md @@ -3,17 +3,58 @@ Origin: [`docs/plans/2026-07-10-001-feat-benchmark-ci-pipeline-plan.md`](../../docs/plans/2026-07-10-001-feat-benchmark-ci-pipeline-plan.md) (and the requirements brainstorm it links). Doctrine in one line: **B/op (`-prof gc` allocation norm) is deterministic on shared runners and is the -only metric that may ever gate; ns/op is directional advice.** +calibrated regression signal; ns/op is directional advice. PR benchmarks +are informational comments, never merge gates.** ## The pieces | Piece | What it does | |---|---| -| `bench_tools.py` | All pipeline logic (mapping, diff, rendering). `python3 -m unittest discover .github/bench` runs its suite; both workflows run it as their first step. | -| `bench-pr.yml` | Same-job A/B on same-repo PRs touching mapped paths: merge-base run, head run, sticky comment. `perf:full` label ⇒ full suite. `workflow_dispatch mode=aa` ⇒ noise calibration. | +| `bench_tools.py` | Mapping, diff, rendering. `python3 -m unittest discover .github/bench` runs its suite and the PR-planning tests; both workflows run it as their first step. | +| `pr_bench.py` | Trusted PR resolution, immutable SHA planning, and freshness check before posting. | +| `bench-pr.yml` | After successful PR CI, runs on the default branch: same-VM merge-base vs exact PR-head comparison, then sticky comment. No benchmark check is attached to the PR head. `perf:full` ⇒ full suite. Manual A/B reruns and A/A calibration remain available. | | `bench-sweep.yml` | Nightly-if-changed + release-tag full sweep. One atomic bot commit to main (`[skip ci]`) carrying `BENCHMARKS.md` + an append to `site/laika-static/bench/series.jsonl`; attaches `jmh-results-.json` to releases. | | `site/laika-static/bench/index.html` | Static history chart, shipped in-tree; Laika copies it (and the series) verbatim into the docs site, so it serves at `/bench/` on the existing Cloudflare Pages deployment — refreshed at each site deploy (preview per main push, production per `v*` tag). | -| `thresholds.json` | **Absent = advisory (current state).** Present = the B/op gate is live. | +| `thresholds.json` | Absent = no calibrated allocation warnings (current state). Present = highlight B/op threshold breaches in the comment, still non-blocking. The local `diff` CLI retains exit 3 for callers that want a gate. | + +## Automatic reports and manual reruns + +`workflow_run` listens for a successful **Continuous Integration** run +whose event was `pull_request`. Push CI runs, fork PRs, closed PRs, stale +head SHAs, and PRs not targeting `main` are ignored. GitHub sometimes omits +`workflow_run.pull_requests`, so resolution uses the same-repository head +branch and verifies the exact triggering head SHA against the live PR. + +Planning and publishing use the workflow's default-branch SHA. Measurement +uses the exact PR head (not a moving branch or synthetic merge ref), compared +with its merge-base against the PR base. Both runs still share one VM and +the existing reduced JMH profile. Docs/site/tests-only changes skip unless +`perf:full` is set. Adding that label **after** CI completes does not trigger +another workflow; dispatch a rerun: + +```sh +gh workflow run bench-pr.yml --ref main -f pr=129 +gh workflow run bench-pr.yml --ref main -f pr=129 -f filter='.*LensBench.*' +gh workflow run bench-pr.yml --ref main -f mode=aa +``` + +Use the default branch in the Actions UI too. A/A benchmarks that branch's +workflow SHA twice and writes a noise report to the summary and artifact; +leave `pr` blank. Dispatching the workflow from a feature branch is rejected +to avoid attaching a long-running check to that branch. + +The comment includes its head SHA and a link to the benchmark run. Immediately +before posting, the publisher rechecks that the PR is open, same-repository, +targets `main`, and still has that SHA. Obsolete results remain in Actions +artifacts but cannot overwrite the current revision's comment. Different head +revisions can finish independently; duplicate measurements of the same PR/head +cancel one another. Cancelled measurements do not publish a failure comment. + +Bench/build/tooling failures produce an explicit ❌ comment and a failed +**detached** Actions run; threshold breaches only highlight the result. +Neither blocks the PR. This workflow must first be on the default branch for +`workflow_run` or manual dispatch to work. Existing PR-head checks from older +workflow runs remain historical records. ## Running the tool locally @@ -47,9 +88,9 @@ but never silently skip. When adding a module or bench class: update the dict; `validate-mapping` (run by both workflows) fails loudly on stale entries, and the unit tests pin the semantics. -## Calibration and flipping the gate (advisory → gating) +## Calibration and enabling allocation warnings -1. Dispatch **Benchmark A/B** with `mode=aa` (same ref twice) at least 3 +1. Dispatch **Benchmark A/B** from `main` with `mode=aa` (same SHA twice) at least 3 times; each posts a noise report to the run summary and artifact. 2. B/op floor must be ~0. If it isn't: first pin the fork JVM's allocation ergonomics — adaptive TLAB sizing makes `gc.alloc.rate.norm` @@ -62,7 +103,7 @@ entries, and the unit tests pin the semantics. see step 4. 3. Commit `.github/bench/thresholds.json`, e.g. `{"bop_regression_pct": 1.0, "bop_min_delta_bytes": 16}` — quantile - data from the reports, not guesses. + data from the reports, not guesses. This enables warnings, not PR gating. 4. **Profile binding (R5):** any change to `JMH_PROFILE`, the JDK, or the runner image invalidates calibration — delete `thresholds.json` in the same PR and recalibrate before restoring it. @@ -79,9 +120,17 @@ recorded in the provenance (`--jmh-params` / `--profile`). ## Credentials and trust boundaries (R15) -- `bench-pr.yml` executes PR code ⇒ same-repo PRs only, token holds only - `contents: read` + `pull-requests: write`; the sticky-comment action is - SHA-pinned. Fork PRs get no run/comment (deploy-site.yml caveat). +- `bench-pr.yml` accepts same-repo PRs only. Its planning job reads Git + objects with a read-only token and never executes PR code. The measurement + job executes PR code with only `contents: read`, no persisted checkout + credentials, and restore-only caches (no PR cache saves on the default + branch). It uploads raw JMH JSON only. +- A separate publisher checks out trusted default-branch tooling, renders + that JSON as data, and holds `pull-requests: write` for the SHA-pinned + sticky-comment action. PR number and SHAs come from the trusted plan, not + the artifact; artifacts cannot supply executable tooling or publication + targets. Threshold configuration also comes from that trusted checkout. + Fork PRs get no run/comment. - `bench-sweep.yml` executes trusted main/tag code only and is the sole holder of `contents: write` and the sole writer of the series file. - Bot pushes use the plain `GITHUB_TOKEN` (proven by diff --git a/.github/bench/bench_tools.py b/.github/bench/bench_tools.py index b78b2063..7a35a07f 100644 --- a/.github/bench/bench_tools.py +++ b/.github/bench/bench_tools.py @@ -291,7 +291,7 @@ def comment_md(deltas: dict | None, failure: str | None) -> str: if deltas["gate"]["enabled"] and deltas["gate"]["violations"]: lines += [ - "### 🚨 B/op gate: " + "### 🚨 B/op regression exceeds calibrated threshold: " + ", ".join(esc(v) for v in deltas["gate"]["violations"]), "", ] diff --git a/.github/bench/pr_bench.py b/.github/bench/pr_bench.py new file mode 100644 index 00000000..0caf6fe8 --- /dev/null +++ b/.github/bench/pr_bench.py @@ -0,0 +1,160 @@ +"""Trusted planning and freshness checks for detached PR benchmarks. + +Runs only on the default-branch checkout, never alongside PR build code. +The benchmark's artifact is data, not a source of PR numbers or SHAs. +""" + +import argparse +import itertools +import json +import os +import pathlib +import re +import subprocess +import urllib.parse +import urllib.request + +import bench_tools + + +class GitHub: + def __init__(self, repository): + self.repository = repository + + def get(self, path, **query): + url = f"{os.environ.get('GITHUB_API_URL', 'https://api.github.com')}/repos/{self.repository}/{path}" + if query: + url += "?" + urllib.parse.urlencode(query) + request = urllib.request.Request(url, headers={ + "Authorization": f"Bearer {os.environ['GH_TOKEN']}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }) + with urllib.request.urlopen(request, timeout=30) as response: + return json.load(response) + + def pulls(self, branch): + # workflow_run.pull_requests can be empty even for same-repo PRs. + for page in itertools.count(1): + batch = self.get("pulls", state="open", base="main", + head=f"{self.repository.split('/')[0]}:{branch}", + per_page=100, page=page) + yield from batch + if len(batch) < 100: + return + + +def current_pr(pr, repository, head=None): + """One rule shared by planning and the last check before commenting.""" + return ( + pr.get("state") == "open" + and pr.get("base", {}).get("ref") == "main" + and (pr.get("head", {}).get("repo") or {}).get("full_name") == repository + and (head is None or pr["head"]["sha"] == head) + ) + + +def resolve(event_name, event, github, workflow_sha): + """Return (PR or None, mode, filter), or None when no run is eligible.""" + if event_name == "workflow_run": + run = event["workflow_run"] + if (run.get("event") != "pull_request" or run.get("conclusion") != "success" + or (run.get("head_repository") or {}).get("full_name") != github.repository): + return None + candidates = [ + pr for pr in github.pulls(run["head_branch"]) + if current_pr(pr, github.repository, run["head_sha"]) + ] + if len(candidates) != 1: + return None + return candidates[0], "ab", "" + + if event_name != "workflow_dispatch": + raise ValueError(f"Unsupported event: {event_name}") + inputs = event.get("inputs", {}) + mode = inputs.get("mode", "ab") + number = inputs.get("pr", "").strip() + regex = inputs.get("filter", "") + if mode == "aa": + if number: + raise ValueError("A/A calibration uses the default branch; leave pr blank") + checked_sha(workflow_sha) + return None, mode, regex + if mode != "ab" or not number.isdecimal() or int(number) < 1: + raise ValueError("A/B requires a positive PR number; use mode=aa for calibration") + pr = github.get(f"pulls/{int(number)}") + if not current_pr(pr, github.repository): + return None + return pr, mode, regex + + +def checked_sha(value): + if not re.fullmatch(r"[0-9a-f]{40}", value): + raise ValueError("Expected an immutable Git SHA") + return value + + +def git(*args): + return subprocess.check_output(["git", *args], text=True).strip() + + +def plan(request, workflow_sha): + if request is None: + return {"run": "false"} + pr, mode, override = request + head = checked_sha(pr["head"]["sha"] if pr else workflow_sha) + if mode == "aa": + base = head + regex = override or "FULL" + else: + target = checked_sha(pr["base"]["sha"]) + git("fetch", "--no-tags", "origin", head, target) + base = checked_sha(git("merge-base", target, head)) + full = any(label["name"] == "perf:full" for label in pr.get("labels", [])) + regex = override or ("FULL" if full else bench_tools.affected( + git("diff", "--name-only", f"{base}...{head}").splitlines())) + return { + "run": "true" if regex else "false", + "pr": str(pr["number"]) if pr else "", + "mode": mode, + "base": base, + "head": head, + "regex": r".*Bench\..*" if regex == "FULL" else regex, + } + + +def write_outputs(values, path): + # Filters are operator input, not executable shell fragments. Reject + # newlines rather than allowing extra workflow outputs to be injected. + lines = [] + for key, value in values.items(): + if "\n" in value or "\r" in value: + raise ValueError("Workflow outputs must be single-line values") + lines.append(f"{key}={value}\n") + with pathlib.Path(path).open("a", encoding="utf-8") as output: + output.writelines(lines) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("command", choices=["plan", "check"]) + args = parser.parse_args() + github = GitHub(os.environ["GITHUB_REPOSITORY"]) + if args.command == "plan": + event = json.loads(pathlib.Path(os.environ["GITHUB_EVENT_PATH"]).read_text()) + default_ref = f"refs/heads/{event['repository']['default_branch']}" + if os.environ["GITHUB_REF"] != default_ref: + raise ValueError("Dispatch the benchmark workflow from the default branch") + values = plan(resolve(os.environ["GITHUB_EVENT_NAME"], event, github, + os.environ["GITHUB_SHA"]), os.environ["GITHUB_SHA"]) + else: + number = int(os.environ["PR_NUMBER"]) + head = checked_sha(os.environ["HEAD_SHA"]) + values = {"current": str(current_pr( + github.get(f"pulls/{number}"), github.repository, head)).lower()} + write_outputs(values, os.environ["GITHUB_OUTPUT"]) + print(json.dumps(values)) + + +if __name__ == "__main__": + main() diff --git a/.github/bench/test_bench_tools.py b/.github/bench/test_bench_tools.py index 78fbdd18..f1a1810c 100644 --- a/.github/bench/test_bench_tools.py +++ b/.github/bench/test_bench_tools.py @@ -187,12 +187,14 @@ def test_untrusted_names_are_escaped(self): self.assertNotIn("