From c2f1c1e75f22c263de780819556b44b0242d57a8 Mon Sep 17 00:00:00 2001 From: Andrej Glavic Date: Wed, 2 Sep 2026 14:59:31 +0000 Subject: [PATCH] Bug 2065580 - Add claude skill for how to triage and resolve performance regressions r=kshampur,ai4dev-reviewers,suhaib We previously added a "perftest" skill which helps you run a performance test. A recent event where a person struggled to resolve a regression made it clear we need to do more, we need a performance regression triage skill. This skill should help all engineers resolve regressions whenever they appear faster(which is useful now that our release schedule is doubled) I included a test set to ensure that the "perftest" and "perf-regression-triage" skills do not get loaded too often and that if it's just a question on how to run a test only the perftest skill gets loaded. Differential Revision: https://phabricator.services.mozilla.com/D320525 --- .../skills/perf-regression-triage/SKILL.md | 137 ++++++++++++++++++ .../references/close-out.md | 70 +++++++++ .../references/confirm.md | 104 +++++++++++++ .../references/iterate.md | 81 +++++++++++ .../tests/trigger-tests.sh | 79 ++++++++++ .agents/skills/perftest/SKILL.md | 17 ++- .../skills/perf-regression-triage/SKILL.md | 137 ++++++++++++++++++ .../references/close-out.md | 70 +++++++++ .../references/confirm.md | 104 +++++++++++++ .../references/iterate.md | 81 +++++++++++ .../tests/trigger-tests.sh | 79 ++++++++++ .claude/skills/perftest/SKILL.md | 17 ++- 12 files changed, 962 insertions(+), 14 deletions(-) create mode 100644 .agents/skills/perf-regression-triage/SKILL.md create mode 100644 .agents/skills/perf-regression-triage/references/close-out.md create mode 100644 .agents/skills/perf-regression-triage/references/confirm.md create mode 100644 .agents/skills/perf-regression-triage/references/iterate.md create mode 100755 .agents/skills/perf-regression-triage/tests/trigger-tests.sh create mode 100644 .claude/skills/perf-regression-triage/SKILL.md create mode 100644 .claude/skills/perf-regression-triage/references/close-out.md create mode 100644 .claude/skills/perf-regression-triage/references/confirm.md create mode 100644 .claude/skills/perf-regression-triage/references/iterate.md create mode 100755 .claude/skills/perf-regression-triage/tests/trigger-tests.sh diff --git a/.agents/skills/perf-regression-triage/SKILL.md b/.agents/skills/perf-regression-triage/SKILL.md new file mode 100644 index 000000000000..c78e56c540e2 --- /dev/null +++ b/.agents/skills/perf-regression-triage/SKILL.md @@ -0,0 +1,137 @@ +--- +name: perf-regression-triage +description: > + Handle a Perfherder performance regression bug end to end: read the alert bug, confirm + whether the regression is real, find the cause, and iterate to a fix. Use when the user + has a Perfherder/browsertime regression bug (the "N% ... regression on " + bugs filed in Testing::Performance), needs to establish whether an alert summary is a + real regression, is interpreting a PerfCompare baseRev/newRev comparison, or asks what + to do about a patch of theirs that regressed a benchmark. + Not for simply running perf tests or pushing them to try — including running an alert + summary's tests with `mach try perf --alert` (use perftest) — nor for choosing or + writing perf tests (use perftest), analyzing a profile you already have in hand + (use profiler-analysis), SpiderMonkey microbenchmarks (use js-perf-investigation), or + non-performance regressions such as failing tests, crashes, or build bustage. +allowed-tools: + - Bash(./mach try perf --help:*) + - Bash(./mach try perf --no-push:*) + - Bash(treeherder-cli:*) + - Bash(profiler-cli:*) + - Bash(git log:*) + - Bash(git show:*) + - Bash(git status:*) + - Bash(jj log:*) + - Bash(hg log:*) + - mcp__moz__get_bugzilla_bug + - Read + - Grep + - Glob +--- + +# Triaging a Perfherder regression bug + +## Tooling + +Three tools cover the three kinds of evidence in a regression bug. Use them rather than +fetching URLs by hand — `WebFetch` on a Treeherder or profiler URL returns a UI shell with +no data in it. + +- **The regression bug** — `mcp__moz__get_bugzilla_bug`, or the `@moz:bugzilla://bug/{id}` + resource. There is no Bugzilla CLI; this is the supported path. +- **Push and job data** — `treeherder-cli `. Use `--perf` for performance and resource + data, `--repo autoland` to inspect the culprit push, and `--watch --notify` to wait on a + running try push instead of polling it. +- **Before/After profiles** — `profiler-cli`, driven through the **profiler-analysis** + skill. If it is not installed: `npm install -g @firefox-devtools/profiler-cli@latest`. + +`profiler-analysis` owns the profiler-cli protocol (run `profiler-cli guide` first, stop +the daemon when done). Hand profiles to that skill instead of reimplementing it here. + +The regression policy gives the patch author **3 business days** to acknowledge and start +investigating before the patch may be backed out. Establish early whether that clock is +running, and tell the user if it is close to expiring. + +## Step 1: read the bug + +Fetch it with `@moz:bugzilla://bug/{id}`. Every Perfherder alert bug carries the same +machine-readable payload. Pull out all of it before doing anything else: + +- **Alert summary ID** — the `perfherder/alerts?id=NNNNN` link. Feeds `--alert` in step 2. +- **Culprit push revision** — the `pushloghtml?changeset=...` link in comment 0. +- **Base / new revisions** — `baseRev=` and `newRev=` in the PerfCompare link. +- **Regressed tests** — first column of the alert table. +- **Platform and options** — e.g. `linux2404-64-shippable`, `fission webrender`. +- **Before/After profiles** — the profiler.firefox.com links in the last column. + +The culprit push usually contains several patches. Inspect it directly rather than reading +the pushlog HTML: + +``` +treeherder-cli --repo autoland --perf +``` + +Identify which patch is the user's and which files it touched (`git show ` / the +linked Phabricator revision). If the push has multiple candidate patches and it is not +obvious which is responsible, say so — the confirmation push below tests the user's patch +specifically, which is what settles it. + +Summarize for the user: how big the regression is, which tests, which platform, and what +the patch changed. Then move to confirmation. + +## Step 2: confirm it + +Do this before any investigation. A meaningful fraction of alerts do not reproduce. + +The whole confirmation is **one command**, not two manual pushes. `--alert` runs exactly +the tests in the alert summary and compares your working revision against the base revision +your patch sits on, pushing both sides for you: + +``` +./mach try perf --alert --rebuild 10 +``` + +Read `references/confirm.md` before running it — it covers getting the local repo onto the +right base revision, how many retriggers are actually needed, the pgo/shippable trap, and +how to read the result. + +**Never push to try without explicit approval from the user.** Show the exact command, say +roughly what it will cost in CI, and wait. + +Then branch on the outcome: + +- **Reproduces** — continue to step 3. +- **Does not reproduce** — do not start optimizing. Go to `references/close-out.md`; this + is likely noise or an unrelated patch in the same push. +- **Ambiguous / overlapping distributions** — more retriggers, or narrow to the single + most-regressed test. See `references/confirm.md`. + +## Step 3: find the cause + +Start with the Before/After profiles already linked in the bug — they are free and +specific to the regressed test. Hand them to the **profiler-analysis** skill rather than +fetching them yourself; `WebFetch` on a profiler URL only retrieves the UI shell. + +Compare against what the patch actually changed. Most Perfherder regressions on +speedometer-class benchmarks come from work added to a hot path, a lost fast path, extra +allocation, or added main-thread sync work. + +## Step 4: iterate to a fix + +Read `references/iterate.md`. The loop is change → narrow try push → check, and the entire +point of that file is keeping each round cheap: narrow the test set, drop to one platform, +and only re-run the full alert set for the final confirmation. + +## Step 5: close out + +Read `references/close-out.md` for the possible resolutions, the bug fields to set, and +who to talk to when the right answer is "this regression is acceptable." + +## Cost discipline + +This skill exists partly to keep regression work cheap, in CI and in tokens. + +- One push per question. `--alert` gives base and new together; do not push twice. +- Confirm broadly once, then iterate narrowly. Full alert-set reruns are for the final + check only. +- Do not read the reference files up front. Read the one for the step you are on. +- Read profiles through profiler-analysis, not by downloading them into context. diff --git a/.agents/skills/perf-regression-triage/references/close-out.md b/.agents/skills/perf-regression-triage/references/close-out.md new file mode 100644 index 000000000000..f5978210149f --- /dev/null +++ b/.agents/skills/perf-regression-triage/references/close-out.md @@ -0,0 +1,70 @@ +# Closing out a perf regression bug + +Every path here ends with a comment in the bug. The perf sheriffs track these, and the +regression policy clock keeps running until the bug reflects reality. + +## Acknowledge early, regardless of outcome + +The policy gives **3 business days** from the bug being filed to acknowledge and begin +investigating, after which the patch may be backed out. If the user has not commented yet +and the bug is more than a day or two old, say so and draft the acknowledgement before +anything else. A one-line "looking at this, confirmation push running" resets the social +clock even when you have no answer yet. + +## Outcomes + +### Confirmed and fixed + +- Post the final unnarrowed PerfCompare link from `iterate.md` showing the alert set back + at base. +- Reference the fix bug or Phabricator revision. +- Resolve **FIXED** once the fix lands. If the fix is a separate bug, leave this one open + and blocked on it rather than resolving early. + +### Confirmed, not going to fix + +This is a legitimate outcome — a correctness fix or a feature can be worth a few percent — +but it is **not the patch author's call alone**. Do not resolve WONTFIX unilaterally. + +- Comment with: the confirmed magnitude, why the patch is worth it, and what was tried. +- Needinfo the perf sheriff named in comment 0 of the alert bug, and raise it in + [#perf-help](https://mozilla.enterprise.slack.com/archives/C03U19JCSFQ) on Slack or + [#perftest:mozilla.org](https://matrix.to/#/#perftest:mozilla.org) on Matrix. +- Let them set the resolution. + +### Not reproduced + +The confirmation push showed no regression, or the distributions overlapped completely. + +- Post the PerfCompare link and state the measured delta versus the reported one. +- Say which of these it looks like: + - **Noise** — the alert fired on variance; the confirmation separates cleanly at base. + - **A different patch in the same push** — your patch isolated cleanly and showed + nothing. Name the other candidates from the pushlog so the sheriffs can redirect. + - **Infrastructure or environment** — a machine pool change, a test harness change, or a + dependency bump landing around the same time. +- Resolve **INVALID** for noise or an infra artifact. For a wrong-patch attribution, leave + it open and needinfo the sheriff rather than resolving — the regression is real, just not + yours. + +### Still ambiguous after two confirmation rounds + +Do not keep pushing. Comment with both PerfCompare links, state that the effect is inside +the noise band at the retrigger counts tried, and needinfo the perf sheriff. They have +history on which suites and platforms are chronically noisy and can often resolve it +without more CI. + +## Bug fields + +- **Severity / priority** — set if still `--`. Match the magnitude: a 10%+ regression on a + headline benchmark like speedometer3 is not S4. +- **`regressed_by`** — should already point at the culprit push. Fix it if the confirmation + push identified a different patch. +- **Assignee** — if the user is not the right owner (their patch isolated clean), unassign + rather than silently sitting on it. +- **Keywords** — `perf` and `regression` are usually set by the filer already. + +## Draft, do not post + +Write the comment and show it to the user. Never post to Bugzilla, change bug state, or +needinfo anyone without their explicit approval. diff --git a/.agents/skills/perf-regression-triage/references/confirm.md b/.agents/skills/perf-regression-triage/references/confirm.md new file mode 100644 index 000000000000..3c54aa053bd7 --- /dev/null +++ b/.agents/skills/perf-regression-triage/references/confirm.md @@ -0,0 +1,104 @@ +# Confirming the regression + +Goal: one command that answers "does this patch actually cause the alert." Everything +here is about getting a trustworthy answer on the first attempt, because a second +confirmation round costs another few hours and a lot of CI. + +## Get the local repo into the right shape + +`--alert` compares your working revision against **the base revision your patches sit on** +in the local repository. The comparison is only meaningful if that base matches the alert. + +1. Take `baseRev` from the PerfCompare link in the bug — that is the push immediately + before the culprit. +2. Update to it, then apply the user's patch on top so exactly one patch separates the two + sides. If the user still has the patch as a local commit, rebase it onto `baseRev`. +3. Confirm with `git log --oneline -3` (or `jj log`) that the patch is a single commit + directly on top of the base. + +If the culprit push contained several patches and only one is the user's, this setup is +what isolates it. That is the point: the alert blames a push, this push blames a patch. + +Verify the task selection without submitting anything: + +``` +./mach try perf --no-push --alert --rebuild 10 +``` + +`--no-push` prints the calculated task selection and changes nothing. Keep it immediately +after `./mach try perf` — the skill's `allowed-tools` entry is the prefix +`Bash(./mach try perf --no-push:*)`, so putting the flag anywhere later means it no longer +matches and the dry run prompts for permission like a real push. Show the user the task +list and the command before asking to push. + +## The push + +``` +./mach try perf --alert --rebuild 10 -m "Confirm bug perf regression" +``` + +Do not submit base and new yourself. This one command pushes both sides for you. + +**Never submit this without the user's explicit approval.** + +### Retriggers + +`--rebuild` accepts 1-20. Use **10**. That is the floor for separating a small real shift +from run-to-run variance on speedometer-class tests, and it covers the 3-10% range where +most alerts land. + +Go higher — 12 to 15 — only for a sub-3% regression or a suite already known to be noisy. +Do not go lower to save CI: a thin run routinely produces an ambiguous result, and the +second round it forces costs far more than the retriggers you skipped. + +### Do not use `--non-pgo` here + +Alerts are almost always detected on shippable/pgo builds (`linux2404-64-shippable` in the +alert table). `--non-pgo` builds faster but is a different optimization configuration, and +a regression can appear or vanish across that boundary. Confirm on the same configuration +the alert fired on. `--non-pgo` belongs in the iteration loop, not here. + +### Narrowing + +If the alert lists 18 tests, `--alert` runs all of them. That is correct for confirmation — +you want to know the true blast radius, and a fix that helps one test can hurt another. + +Narrow only if the user is explicitly trading coverage for turnaround: + +``` +./mach try perf --alert --tests speedometer3 --platforms linux --rebuild 10 +``` + +Say plainly which tests you dropped. Silent narrowing turns "confirmed" into a claim the +push does not support. + +## Reading the result + +`mach try perf` prints a PerfCompare link when it finishes. That is the primary artifact — +open it, or give it to the user. For the raw job state: + +``` +treeherder-cli --perf +treeherder-cli --watch --notify # if it is still running +``` + +Judge it on three things, in order: + +1. **Direction and size.** Does the delta match the bug's reported magnitude? A bug + claiming 12% that reproduces at 1% is not confirmation. +2. **Overlap.** With 10 retriggers per side, do the two distributions actually separate? A + large mean delta with heavily overlapping runs is noise. +3. **Consistency across the alert set.** Real regressions from one patch usually move a + coherent group of subtests, not one outlier out of eighteen. + +PerfCompare's own confidence indicator is a reasonable tiebreaker, but the overlap check +matters more than the label. + +## Outcomes + +- **Confirmed** — record the measured delta per test in the bug (it becomes the target the + fix has to clear). Continue to step 3 in SKILL.md. +- **Not reproduced** — do not start optimizing. Go to `close-out.md`. +- **Ambiguous** — one more round at higher `--rebuild`, narrowed to the two or three + most-regressed subtests. If it is still ambiguous, that is itself the finding; take it to + `close-out.md` and the perf sheriffs rather than burning more CI. diff --git a/.agents/skills/perf-regression-triage/references/iterate.md b/.agents/skills/perf-regression-triage/references/iterate.md new file mode 100644 index 000000000000..9ef960603676 --- /dev/null +++ b/.agents/skills/perf-regression-triage/references/iterate.md @@ -0,0 +1,81 @@ +# Iterating to a fix + +The loop is: hypothesis, minimal change, narrow try push, check. Each round takes hours of +wall clock and real CI capacity, so the value is almost entirely in making each round +answer one clean question. + +## Before the first round + +Have these written down: + +- The confirmed delta per test, from `confirm.md`. This is the number the fix must clear. +- One or two **target tests** — the most-regressed subtests from the alert. These are the + signal you iterate against. +- A specific hypothesis about the mechanism, from the Before/After profiles (via the + **profiler-analysis** skill) and the patch diff. + +Iterating without a mechanism hypothesis turns into guess-and-push, which is the single +most expensive failure mode here. + +## The iteration push + +Deliberately not the confirmation command. Narrow hard: + +``` +./mach try perf --alert \ + --tests \ + --platforms linux \ + --non-pgo \ + --rebuild 6 \ + -m "bug perf fix attempt N: " +``` + +What each narrowing buys, and what it costs: + +- `--tests ` — cuts the task count sharply. Cost: you stop seeing collateral + movement in the other regressed subtests. +- `--platforms linux` — one platform instead of all. Cost: platform-specific behaviour is + invisible. Use the platform the alert fired on. +- `--non-pgo` — noticeably faster builds. Cost: different optimization configuration, so + treat the result as **directional only**. A fix that works here still has to be proven on + shippable. +- `--rebuild 5` or `6` — deliberately thinner than the confirmation run's 10. Enough to see + whether a change moved the number at all, which is the only question a round needs to + answer. Cost: a marginal result here is not trustworthy on its own, so do not treat a + small improvement as a fix until the unnarrowed run below confirms it at 10. + +Put the hypothesis in `-m`. Three attempts later it is the only thing that tells you what +each push was testing. + +Every push still needs the user's explicit approval. + +## Between rounds + +- Change **one thing** per round. Two changes in one push means an ambiguous result and a + wasted round. +- If a round shows no movement, suspect the hypothesis before suspecting the measurement — + especially if the change was small and the noise band is wide. +- Prefer changes that restore the old behaviour on the hot path over changes that add new + optimization. Reverting a fast path you removed is easy to reason about; a new + optimization needs its own justification and its own review. +- If three rounds produce nothing, stop pushing and go back to the profiles. Consider + whether the honest answer is a partial fix or an accepted tradeoff — see + `close-out.md`. +- Want a profile of your own attempt rather than the ones in the bug: add `--profile` to + the push, then analyze it through **profiler-analysis**. + +## Final verification + +A narrowed `--non-pgo` result is not sufficient to close the bug. Once a round looks good, +run the confirmation command again, unnarrowed, with the fix applied: + +``` +./mach try perf --alert --rebuild 10 -m "bug perf fix verification" +``` + +This must show the full alert set back at or near the base numbers. Check specifically that +no other subtest in the set got worse — a fix that trades one speedometer subtest for +another is not a fix, and the perf sheriffs will catch it. + +Link that PerfCompare result in the bug and on the review. It is the evidence the fix +works, and it is what lets a reviewer approve a perf-motivated change quickly. diff --git a/.agents/skills/perf-regression-triage/tests/trigger-tests.sh b/.agents/skills/perf-regression-triage/tests/trigger-tests.sh new file mode 100755 index 000000000000..dc881fe57928 --- /dev/null +++ b/.agents/skills/perf-regression-triage/tests/trigger-tests.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# Cold-tests which skill fires for a set of prompts, to catch trigger regressions on the +# perftest / perf-regression-triage / profiler-analysis boundary. +# +# ./trigger-tests.sh [results-dir] +# +# Exits 0 if every case routes as expected, 1 otherwise. Each run spawns a fresh headless +# session granted only the Skill tool: granting Read as well lets the model reach skill +# content without invoking Skill, which silently defeats the measurement. +# +# Advisory, not a gate. This measures model behaviour, so a case can fail for reasons +# unrelated to the skill descriptions (model update, nondeterminism). Re-run before +# concluding an edit broke routing, and do not wire it into CI. +# +# Add a case as "name|expected-skill|prompt", using when nothing should fire. + +set -uo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../../.." && pwd)" +RESULTS_DIR="${1:-$REPO_ROOT/artifacts/skill-trigger-tests/$(date +%Y%m%dT%H%M%S)}" + +# Project skills only load for sessions rooted in the repo; without this every case +# silently reports when the script is invoked from elsewhere. +cd "$REPO_ROOT" || { echo "cannot cd to $REPO_ROOT" >&2; exit 2; } + +CASES=( + "pos-bug-live-framing|perf-regression-triage|https://bugzilla.mozilla.org/show_bug.cgi?id=2034476 my patch caused this regression, what do I do now" + "pos-alert-no-url|perf-regression-triage|Perfherder filed an alert saying my patch regressed speedometer3 by 12% on linux2404-64-shippable. Alert summary 49649. What now?" + "pos-perfcompare|perf-regression-triage|what does this PerfCompare result mean, my patch looks slower" + "neg-raptor-local|perftest|how do I run raptor speedometer3 locally" + "neg-alert-tests-on-try|perftest|how do I run the tests from alert summary 49649 on try" + "neg-profiler-link|profiler-analysis|can you analyze https://share.firefox.dev/4xbihh8" + "neg-mochitest-failure||this mochitest started failing on autoland" +) + +command -v claude >/dev/null || { echo "claude not found in PATH" >&2; exit 2; } +command -v rg >/dev/null || { echo "rg not found in PATH; run ./mach bootstrap" >&2; exit 2; } + +mkdir -p "$RESULTS_DIR" +echo "Running ${#CASES[@]} cases -> $RESULTS_DIR" +echo + +for entry in "${CASES[@]}"; do + name="${entry%%|*}" + rest="${entry#*|}" + prompt="${rest#*|}" + claude -p "$prompt" \ + --allowedTools Skill \ + --output-format stream-json \ + --verbose \ + > "$RESULTS_DIR/$name.jsonl" 2>&1 & +done +wait + +failures=0 +for entry in "${CASES[@]}"; do + name="${entry%%|*}" + rest="${entry#*|}" + expected="${rest%%|*}" + + fired=$(rg -o '"name":"Skill","input":\{"skill":"[^"]*"' "$RESULTS_DIR/$name.jsonl" 2>/dev/null \ + | sed 's/.*"skill":"//; s/"$//' | sort -u | paste -sd, -) + [ -z "$fired" ] && fired="" + + if [ "$fired" = "$expected" ]; then + printf 'PASS %-24s %s\n' "$name" "$fired" + else + printf 'FAIL %-24s expected %s, got %s\n' "$name" "$expected" "$fired" + failures=$((failures + 1)) + fi +done + +echo +if [ "$failures" -eq 0 ]; then + echo "All ${#CASES[@]} cases routed as expected." + exit 0 +fi +echo "$failures of ${#CASES[@]} cases mis-routed. Transcripts in $RESULTS_DIR" +exit 1 diff --git a/.agents/skills/perftest/SKILL.md b/.agents/skills/perftest/SKILL.md index 261f515ef6ec..4c2a845a4935 100644 --- a/.agents/skills/perftest/SKILL.md +++ b/.agents/skills/perftest/SKILL.md @@ -2,13 +2,16 @@ name: perftest description: > Run Firefox performance tests locally or in CI. Use when the user asks how to run a - perf test, wants to reproduce a performance alert or regression locally, needs the - mach invocation for Raptor, Talos, MozPerftest, AWSY, or browsertime, wants to push - perf tests to try, or mentions `mach try perf`, `mach perftest`, `mach raptor`, - `mach talos-test`, an alert summary ID, PerfCompare, or Compare View. Covers picking - the right harness, finding a test's name, and the local-vs-CI tradeoff. - Not for analyzing an existing Firefox profile (use profiler-analysis) or for - SpiderMonkey/JS engine benchmarking (use js-perf-investigation). + perf test, needs the mach invocation for Raptor, Talos, MozPerftest, AWSY, or + browsertime, wants to push perf tests to try, wants to run the tests belonging to an + alert summary (`mach try perf --alert `), or mentions `mach try perf`, + `mach perftest`, `mach raptor`, or `mach talos-test`. Covers picking the right + harness, finding a test's name, and the local-vs-CI tradeoff. + Not for triaging or root-causing a Perfherder regression bug — confirming whether a + regression is real, identifying the culprit patch, interpreting a PerfCompare result, + or deciding backout vs fix-forward (use perf-regression-triage). Not for analyzing an + existing Firefox profile (use profiler-analysis) or SpiderMonkey/JS engine + benchmarking (use js-perf-investigation). allowed-tools: - Bash(./mach perftest --help:*) - Bash(./mach raptor --help:*) diff --git a/.claude/skills/perf-regression-triage/SKILL.md b/.claude/skills/perf-regression-triage/SKILL.md new file mode 100644 index 000000000000..c78e56c540e2 --- /dev/null +++ b/.claude/skills/perf-regression-triage/SKILL.md @@ -0,0 +1,137 @@ +--- +name: perf-regression-triage +description: > + Handle a Perfherder performance regression bug end to end: read the alert bug, confirm + whether the regression is real, find the cause, and iterate to a fix. Use when the user + has a Perfherder/browsertime regression bug (the "N% ... regression on " + bugs filed in Testing::Performance), needs to establish whether an alert summary is a + real regression, is interpreting a PerfCompare baseRev/newRev comparison, or asks what + to do about a patch of theirs that regressed a benchmark. + Not for simply running perf tests or pushing them to try — including running an alert + summary's tests with `mach try perf --alert` (use perftest) — nor for choosing or + writing perf tests (use perftest), analyzing a profile you already have in hand + (use profiler-analysis), SpiderMonkey microbenchmarks (use js-perf-investigation), or + non-performance regressions such as failing tests, crashes, or build bustage. +allowed-tools: + - Bash(./mach try perf --help:*) + - Bash(./mach try perf --no-push:*) + - Bash(treeherder-cli:*) + - Bash(profiler-cli:*) + - Bash(git log:*) + - Bash(git show:*) + - Bash(git status:*) + - Bash(jj log:*) + - Bash(hg log:*) + - mcp__moz__get_bugzilla_bug + - Read + - Grep + - Glob +--- + +# Triaging a Perfherder regression bug + +## Tooling + +Three tools cover the three kinds of evidence in a regression bug. Use them rather than +fetching URLs by hand — `WebFetch` on a Treeherder or profiler URL returns a UI shell with +no data in it. + +- **The regression bug** — `mcp__moz__get_bugzilla_bug`, or the `@moz:bugzilla://bug/{id}` + resource. There is no Bugzilla CLI; this is the supported path. +- **Push and job data** — `treeherder-cli `. Use `--perf` for performance and resource + data, `--repo autoland` to inspect the culprit push, and `--watch --notify` to wait on a + running try push instead of polling it. +- **Before/After profiles** — `profiler-cli`, driven through the **profiler-analysis** + skill. If it is not installed: `npm install -g @firefox-devtools/profiler-cli@latest`. + +`profiler-analysis` owns the profiler-cli protocol (run `profiler-cli guide` first, stop +the daemon when done). Hand profiles to that skill instead of reimplementing it here. + +The regression policy gives the patch author **3 business days** to acknowledge and start +investigating before the patch may be backed out. Establish early whether that clock is +running, and tell the user if it is close to expiring. + +## Step 1: read the bug + +Fetch it with `@moz:bugzilla://bug/{id}`. Every Perfherder alert bug carries the same +machine-readable payload. Pull out all of it before doing anything else: + +- **Alert summary ID** — the `perfherder/alerts?id=NNNNN` link. Feeds `--alert` in step 2. +- **Culprit push revision** — the `pushloghtml?changeset=...` link in comment 0. +- **Base / new revisions** — `baseRev=` and `newRev=` in the PerfCompare link. +- **Regressed tests** — first column of the alert table. +- **Platform and options** — e.g. `linux2404-64-shippable`, `fission webrender`. +- **Before/After profiles** — the profiler.firefox.com links in the last column. + +The culprit push usually contains several patches. Inspect it directly rather than reading +the pushlog HTML: + +``` +treeherder-cli --repo autoland --perf +``` + +Identify which patch is the user's and which files it touched (`git show ` / the +linked Phabricator revision). If the push has multiple candidate patches and it is not +obvious which is responsible, say so — the confirmation push below tests the user's patch +specifically, which is what settles it. + +Summarize for the user: how big the regression is, which tests, which platform, and what +the patch changed. Then move to confirmation. + +## Step 2: confirm it + +Do this before any investigation. A meaningful fraction of alerts do not reproduce. + +The whole confirmation is **one command**, not two manual pushes. `--alert` runs exactly +the tests in the alert summary and compares your working revision against the base revision +your patch sits on, pushing both sides for you: + +``` +./mach try perf --alert --rebuild 10 +``` + +Read `references/confirm.md` before running it — it covers getting the local repo onto the +right base revision, how many retriggers are actually needed, the pgo/shippable trap, and +how to read the result. + +**Never push to try without explicit approval from the user.** Show the exact command, say +roughly what it will cost in CI, and wait. + +Then branch on the outcome: + +- **Reproduces** — continue to step 3. +- **Does not reproduce** — do not start optimizing. Go to `references/close-out.md`; this + is likely noise or an unrelated patch in the same push. +- **Ambiguous / overlapping distributions** — more retriggers, or narrow to the single + most-regressed test. See `references/confirm.md`. + +## Step 3: find the cause + +Start with the Before/After profiles already linked in the bug — they are free and +specific to the regressed test. Hand them to the **profiler-analysis** skill rather than +fetching them yourself; `WebFetch` on a profiler URL only retrieves the UI shell. + +Compare against what the patch actually changed. Most Perfherder regressions on +speedometer-class benchmarks come from work added to a hot path, a lost fast path, extra +allocation, or added main-thread sync work. + +## Step 4: iterate to a fix + +Read `references/iterate.md`. The loop is change → narrow try push → check, and the entire +point of that file is keeping each round cheap: narrow the test set, drop to one platform, +and only re-run the full alert set for the final confirmation. + +## Step 5: close out + +Read `references/close-out.md` for the possible resolutions, the bug fields to set, and +who to talk to when the right answer is "this regression is acceptable." + +## Cost discipline + +This skill exists partly to keep regression work cheap, in CI and in tokens. + +- One push per question. `--alert` gives base and new together; do not push twice. +- Confirm broadly once, then iterate narrowly. Full alert-set reruns are for the final + check only. +- Do not read the reference files up front. Read the one for the step you are on. +- Read profiles through profiler-analysis, not by downloading them into context. diff --git a/.claude/skills/perf-regression-triage/references/close-out.md b/.claude/skills/perf-regression-triage/references/close-out.md new file mode 100644 index 000000000000..f5978210149f --- /dev/null +++ b/.claude/skills/perf-regression-triage/references/close-out.md @@ -0,0 +1,70 @@ +# Closing out a perf regression bug + +Every path here ends with a comment in the bug. The perf sheriffs track these, and the +regression policy clock keeps running until the bug reflects reality. + +## Acknowledge early, regardless of outcome + +The policy gives **3 business days** from the bug being filed to acknowledge and begin +investigating, after which the patch may be backed out. If the user has not commented yet +and the bug is more than a day or two old, say so and draft the acknowledgement before +anything else. A one-line "looking at this, confirmation push running" resets the social +clock even when you have no answer yet. + +## Outcomes + +### Confirmed and fixed + +- Post the final unnarrowed PerfCompare link from `iterate.md` showing the alert set back + at base. +- Reference the fix bug or Phabricator revision. +- Resolve **FIXED** once the fix lands. If the fix is a separate bug, leave this one open + and blocked on it rather than resolving early. + +### Confirmed, not going to fix + +This is a legitimate outcome — a correctness fix or a feature can be worth a few percent — +but it is **not the patch author's call alone**. Do not resolve WONTFIX unilaterally. + +- Comment with: the confirmed magnitude, why the patch is worth it, and what was tried. +- Needinfo the perf sheriff named in comment 0 of the alert bug, and raise it in + [#perf-help](https://mozilla.enterprise.slack.com/archives/C03U19JCSFQ) on Slack or + [#perftest:mozilla.org](https://matrix.to/#/#perftest:mozilla.org) on Matrix. +- Let them set the resolution. + +### Not reproduced + +The confirmation push showed no regression, or the distributions overlapped completely. + +- Post the PerfCompare link and state the measured delta versus the reported one. +- Say which of these it looks like: + - **Noise** — the alert fired on variance; the confirmation separates cleanly at base. + - **A different patch in the same push** — your patch isolated cleanly and showed + nothing. Name the other candidates from the pushlog so the sheriffs can redirect. + - **Infrastructure or environment** — a machine pool change, a test harness change, or a + dependency bump landing around the same time. +- Resolve **INVALID** for noise or an infra artifact. For a wrong-patch attribution, leave + it open and needinfo the sheriff rather than resolving — the regression is real, just not + yours. + +### Still ambiguous after two confirmation rounds + +Do not keep pushing. Comment with both PerfCompare links, state that the effect is inside +the noise band at the retrigger counts tried, and needinfo the perf sheriff. They have +history on which suites and platforms are chronically noisy and can often resolve it +without more CI. + +## Bug fields + +- **Severity / priority** — set if still `--`. Match the magnitude: a 10%+ regression on a + headline benchmark like speedometer3 is not S4. +- **`regressed_by`** — should already point at the culprit push. Fix it if the confirmation + push identified a different patch. +- **Assignee** — if the user is not the right owner (their patch isolated clean), unassign + rather than silently sitting on it. +- **Keywords** — `perf` and `regression` are usually set by the filer already. + +## Draft, do not post + +Write the comment and show it to the user. Never post to Bugzilla, change bug state, or +needinfo anyone without their explicit approval. diff --git a/.claude/skills/perf-regression-triage/references/confirm.md b/.claude/skills/perf-regression-triage/references/confirm.md new file mode 100644 index 000000000000..3c54aa053bd7 --- /dev/null +++ b/.claude/skills/perf-regression-triage/references/confirm.md @@ -0,0 +1,104 @@ +# Confirming the regression + +Goal: one command that answers "does this patch actually cause the alert." Everything +here is about getting a trustworthy answer on the first attempt, because a second +confirmation round costs another few hours and a lot of CI. + +## Get the local repo into the right shape + +`--alert` compares your working revision against **the base revision your patches sit on** +in the local repository. The comparison is only meaningful if that base matches the alert. + +1. Take `baseRev` from the PerfCompare link in the bug — that is the push immediately + before the culprit. +2. Update to it, then apply the user's patch on top so exactly one patch separates the two + sides. If the user still has the patch as a local commit, rebase it onto `baseRev`. +3. Confirm with `git log --oneline -3` (or `jj log`) that the patch is a single commit + directly on top of the base. + +If the culprit push contained several patches and only one is the user's, this setup is +what isolates it. That is the point: the alert blames a push, this push blames a patch. + +Verify the task selection without submitting anything: + +``` +./mach try perf --no-push --alert --rebuild 10 +``` + +`--no-push` prints the calculated task selection and changes nothing. Keep it immediately +after `./mach try perf` — the skill's `allowed-tools` entry is the prefix +`Bash(./mach try perf --no-push:*)`, so putting the flag anywhere later means it no longer +matches and the dry run prompts for permission like a real push. Show the user the task +list and the command before asking to push. + +## The push + +``` +./mach try perf --alert --rebuild 10 -m "Confirm bug perf regression" +``` + +Do not submit base and new yourself. This one command pushes both sides for you. + +**Never submit this without the user's explicit approval.** + +### Retriggers + +`--rebuild` accepts 1-20. Use **10**. That is the floor for separating a small real shift +from run-to-run variance on speedometer-class tests, and it covers the 3-10% range where +most alerts land. + +Go higher — 12 to 15 — only for a sub-3% regression or a suite already known to be noisy. +Do not go lower to save CI: a thin run routinely produces an ambiguous result, and the +second round it forces costs far more than the retriggers you skipped. + +### Do not use `--non-pgo` here + +Alerts are almost always detected on shippable/pgo builds (`linux2404-64-shippable` in the +alert table). `--non-pgo` builds faster but is a different optimization configuration, and +a regression can appear or vanish across that boundary. Confirm on the same configuration +the alert fired on. `--non-pgo` belongs in the iteration loop, not here. + +### Narrowing + +If the alert lists 18 tests, `--alert` runs all of them. That is correct for confirmation — +you want to know the true blast radius, and a fix that helps one test can hurt another. + +Narrow only if the user is explicitly trading coverage for turnaround: + +``` +./mach try perf --alert --tests speedometer3 --platforms linux --rebuild 10 +``` + +Say plainly which tests you dropped. Silent narrowing turns "confirmed" into a claim the +push does not support. + +## Reading the result + +`mach try perf` prints a PerfCompare link when it finishes. That is the primary artifact — +open it, or give it to the user. For the raw job state: + +``` +treeherder-cli --perf +treeherder-cli --watch --notify # if it is still running +``` + +Judge it on three things, in order: + +1. **Direction and size.** Does the delta match the bug's reported magnitude? A bug + claiming 12% that reproduces at 1% is not confirmation. +2. **Overlap.** With 10 retriggers per side, do the two distributions actually separate? A + large mean delta with heavily overlapping runs is noise. +3. **Consistency across the alert set.** Real regressions from one patch usually move a + coherent group of subtests, not one outlier out of eighteen. + +PerfCompare's own confidence indicator is a reasonable tiebreaker, but the overlap check +matters more than the label. + +## Outcomes + +- **Confirmed** — record the measured delta per test in the bug (it becomes the target the + fix has to clear). Continue to step 3 in SKILL.md. +- **Not reproduced** — do not start optimizing. Go to `close-out.md`. +- **Ambiguous** — one more round at higher `--rebuild`, narrowed to the two or three + most-regressed subtests. If it is still ambiguous, that is itself the finding; take it to + `close-out.md` and the perf sheriffs rather than burning more CI. diff --git a/.claude/skills/perf-regression-triage/references/iterate.md b/.claude/skills/perf-regression-triage/references/iterate.md new file mode 100644 index 000000000000..9ef960603676 --- /dev/null +++ b/.claude/skills/perf-regression-triage/references/iterate.md @@ -0,0 +1,81 @@ +# Iterating to a fix + +The loop is: hypothesis, minimal change, narrow try push, check. Each round takes hours of +wall clock and real CI capacity, so the value is almost entirely in making each round +answer one clean question. + +## Before the first round + +Have these written down: + +- The confirmed delta per test, from `confirm.md`. This is the number the fix must clear. +- One or two **target tests** — the most-regressed subtests from the alert. These are the + signal you iterate against. +- A specific hypothesis about the mechanism, from the Before/After profiles (via the + **profiler-analysis** skill) and the patch diff. + +Iterating without a mechanism hypothesis turns into guess-and-push, which is the single +most expensive failure mode here. + +## The iteration push + +Deliberately not the confirmation command. Narrow hard: + +``` +./mach try perf --alert \ + --tests \ + --platforms linux \ + --non-pgo \ + --rebuild 6 \ + -m "bug perf fix attempt N: " +``` + +What each narrowing buys, and what it costs: + +- `--tests ` — cuts the task count sharply. Cost: you stop seeing collateral + movement in the other regressed subtests. +- `--platforms linux` — one platform instead of all. Cost: platform-specific behaviour is + invisible. Use the platform the alert fired on. +- `--non-pgo` — noticeably faster builds. Cost: different optimization configuration, so + treat the result as **directional only**. A fix that works here still has to be proven on + shippable. +- `--rebuild 5` or `6` — deliberately thinner than the confirmation run's 10. Enough to see + whether a change moved the number at all, which is the only question a round needs to + answer. Cost: a marginal result here is not trustworthy on its own, so do not treat a + small improvement as a fix until the unnarrowed run below confirms it at 10. + +Put the hypothesis in `-m`. Three attempts later it is the only thing that tells you what +each push was testing. + +Every push still needs the user's explicit approval. + +## Between rounds + +- Change **one thing** per round. Two changes in one push means an ambiguous result and a + wasted round. +- If a round shows no movement, suspect the hypothesis before suspecting the measurement — + especially if the change was small and the noise band is wide. +- Prefer changes that restore the old behaviour on the hot path over changes that add new + optimization. Reverting a fast path you removed is easy to reason about; a new + optimization needs its own justification and its own review. +- If three rounds produce nothing, stop pushing and go back to the profiles. Consider + whether the honest answer is a partial fix or an accepted tradeoff — see + `close-out.md`. +- Want a profile of your own attempt rather than the ones in the bug: add `--profile` to + the push, then analyze it through **profiler-analysis**. + +## Final verification + +A narrowed `--non-pgo` result is not sufficient to close the bug. Once a round looks good, +run the confirmation command again, unnarrowed, with the fix applied: + +``` +./mach try perf --alert --rebuild 10 -m "bug perf fix verification" +``` + +This must show the full alert set back at or near the base numbers. Check specifically that +no other subtest in the set got worse — a fix that trades one speedometer subtest for +another is not a fix, and the perf sheriffs will catch it. + +Link that PerfCompare result in the bug and on the review. It is the evidence the fix +works, and it is what lets a reviewer approve a perf-motivated change quickly. diff --git a/.claude/skills/perf-regression-triage/tests/trigger-tests.sh b/.claude/skills/perf-regression-triage/tests/trigger-tests.sh new file mode 100755 index 000000000000..dc881fe57928 --- /dev/null +++ b/.claude/skills/perf-regression-triage/tests/trigger-tests.sh @@ -0,0 +1,79 @@ +#!/usr/bin/env bash +# Cold-tests which skill fires for a set of prompts, to catch trigger regressions on the +# perftest / perf-regression-triage / profiler-analysis boundary. +# +# ./trigger-tests.sh [results-dir] +# +# Exits 0 if every case routes as expected, 1 otherwise. Each run spawns a fresh headless +# session granted only the Skill tool: granting Read as well lets the model reach skill +# content without invoking Skill, which silently defeats the measurement. +# +# Advisory, not a gate. This measures model behaviour, so a case can fail for reasons +# unrelated to the skill descriptions (model update, nondeterminism). Re-run before +# concluding an edit broke routing, and do not wire it into CI. +# +# Add a case as "name|expected-skill|prompt", using when nothing should fire. + +set -uo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../../.." && pwd)" +RESULTS_DIR="${1:-$REPO_ROOT/artifacts/skill-trigger-tests/$(date +%Y%m%dT%H%M%S)}" + +# Project skills only load for sessions rooted in the repo; without this every case +# silently reports when the script is invoked from elsewhere. +cd "$REPO_ROOT" || { echo "cannot cd to $REPO_ROOT" >&2; exit 2; } + +CASES=( + "pos-bug-live-framing|perf-regression-triage|https://bugzilla.mozilla.org/show_bug.cgi?id=2034476 my patch caused this regression, what do I do now" + "pos-alert-no-url|perf-regression-triage|Perfherder filed an alert saying my patch regressed speedometer3 by 12% on linux2404-64-shippable. Alert summary 49649. What now?" + "pos-perfcompare|perf-regression-triage|what does this PerfCompare result mean, my patch looks slower" + "neg-raptor-local|perftest|how do I run raptor speedometer3 locally" + "neg-alert-tests-on-try|perftest|how do I run the tests from alert summary 49649 on try" + "neg-profiler-link|profiler-analysis|can you analyze https://share.firefox.dev/4xbihh8" + "neg-mochitest-failure||this mochitest started failing on autoland" +) + +command -v claude >/dev/null || { echo "claude not found in PATH" >&2; exit 2; } +command -v rg >/dev/null || { echo "rg not found in PATH; run ./mach bootstrap" >&2; exit 2; } + +mkdir -p "$RESULTS_DIR" +echo "Running ${#CASES[@]} cases -> $RESULTS_DIR" +echo + +for entry in "${CASES[@]}"; do + name="${entry%%|*}" + rest="${entry#*|}" + prompt="${rest#*|}" + claude -p "$prompt" \ + --allowedTools Skill \ + --output-format stream-json \ + --verbose \ + > "$RESULTS_DIR/$name.jsonl" 2>&1 & +done +wait + +failures=0 +for entry in "${CASES[@]}"; do + name="${entry%%|*}" + rest="${entry#*|}" + expected="${rest%%|*}" + + fired=$(rg -o '"name":"Skill","input":\{"skill":"[^"]*"' "$RESULTS_DIR/$name.jsonl" 2>/dev/null \ + | sed 's/.*"skill":"//; s/"$//' | sort -u | paste -sd, -) + [ -z "$fired" ] && fired="" + + if [ "$fired" = "$expected" ]; then + printf 'PASS %-24s %s\n' "$name" "$fired" + else + printf 'FAIL %-24s expected %s, got %s\n' "$name" "$expected" "$fired" + failures=$((failures + 1)) + fi +done + +echo +if [ "$failures" -eq 0 ]; then + echo "All ${#CASES[@]} cases routed as expected." + exit 0 +fi +echo "$failures of ${#CASES[@]} cases mis-routed. Transcripts in $RESULTS_DIR" +exit 1 diff --git a/.claude/skills/perftest/SKILL.md b/.claude/skills/perftest/SKILL.md index 261f515ef6ec..4c2a845a4935 100644 --- a/.claude/skills/perftest/SKILL.md +++ b/.claude/skills/perftest/SKILL.md @@ -2,13 +2,16 @@ name: perftest description: > Run Firefox performance tests locally or in CI. Use when the user asks how to run a - perf test, wants to reproduce a performance alert or regression locally, needs the - mach invocation for Raptor, Talos, MozPerftest, AWSY, or browsertime, wants to push - perf tests to try, or mentions `mach try perf`, `mach perftest`, `mach raptor`, - `mach talos-test`, an alert summary ID, PerfCompare, or Compare View. Covers picking - the right harness, finding a test's name, and the local-vs-CI tradeoff. - Not for analyzing an existing Firefox profile (use profiler-analysis) or for - SpiderMonkey/JS engine benchmarking (use js-perf-investigation). + perf test, needs the mach invocation for Raptor, Talos, MozPerftest, AWSY, or + browsertime, wants to push perf tests to try, wants to run the tests belonging to an + alert summary (`mach try perf --alert `), or mentions `mach try perf`, + `mach perftest`, `mach raptor`, or `mach talos-test`. Covers picking the right + harness, finding a test's name, and the local-vs-CI tradeoff. + Not for triaging or root-causing a Perfherder regression bug — confirming whether a + regression is real, identifying the culprit patch, interpreting a PerfCompare result, + or deciding backout vs fix-forward (use perf-regression-triage). Not for analyzing an + existing Firefox profile (use profiler-analysis) or SpiderMonkey/JS engine + benchmarking (use js-perf-investigation). allowed-tools: - Bash(./mach perftest --help:*) - Bash(./mach raptor --help:*)