diff --git a/.agents/skills/perf-review/SKILL.md b/.agents/skills/perf-review/SKILL.md index 8d120f064fd..9ff896bc320 100644 --- a/.agents/skills/perf-review/SKILL.md +++ b/.agents/skills/perf-review/SKILL.md @@ -116,6 +116,11 @@ mechanism is certain or the severity is catastrophic. **routine adoption of a known-better idiom** (migrating to a lower-overhead builder / API / toolkit primitive — no visible downside); or a change that **ships a benchmark/JFR** (well-evidenced — recognize it). One line; a nudge, not a code-pattern finding. +- **Shipped core code is stricter — see the core performance rules in `AGENTS.md`.** + For changes to `dd-trace-core/src/main/` or `internal-api/src/main/`: a performance claim + (in the title, description, or commits) without JMH before/after numbers is a finding, not + a soft nudge. Elsewhere, including those modules' tests, benchmarks and build files, keep + the softer posture above. ## Workflow @@ -137,11 +142,22 @@ git diff $MERGE_BASE --stat git diff $MERGE_BASE --name-status ``` +Also read the claims the change makes. A performance claim can live only in the PR title +or description, which the diff doesn't show: + +```bash +gh pr view --json title,body 2>/dev/null # the branch's PR, if it has one +git log --format='%s%n%b' $MERGE_BASE..HEAD # commit messages +``` + +If the branch has no PR, or `gh` can't reach it, check the commit messages and say in the +summary that the PR text wasn't available, so a claim made only there wasn't checked. + If there are no changes, say so and stop. Otherwise read the diff **and the full content of the modified source files** (not just the hunks) — the interprocedural condition (who calls this, what a helper does, where a hook's sink lands) lives outside the diff window. Ignore the PR description if the user asks for an -independent review. +independent review, except to find performance claims. ### Step 2 — Map the changed code onto hot paths diff --git a/AGENTS.md b/AGENTS.md index a696c26b3da..06a5e7fabb7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -82,6 +82,12 @@ docs/ Developer documentation (see below) - **Technical debt**: run `/techdebt` over branch changes before marking a PR ready to catch code duplication, unnecessary complexity, and dead code (refactor-only, never changes behavior) — see [.agents/skills/techdebt/SKILL.md](.agents/skills/techdebt/SKILL.md). - **Performance**: run `/perf-review` over branch changes before marking a PR ready (advisory, not a merge gate) — see [.agents/skills/perf-review/SKILL.md](.agents/skills/perf-review/SKILL.md). +### Core performance rules + +These rules are strict for shipped runtime code in `dd-trace-core/src/main/` and `internal-api/src/main/`. Elsewhere, including those modules' tests, benchmarks and build files, treat them as advice. + +- **Measure performance claims.** A PR that claims to be faster or to allocate less must include JMH before/after results from the same machine, JDK, JVM flags and benchmark settings, with time and allocation (`-Pjmh.profilers=gc`). Without them, reviewers should flag the claim as unmeasured and the author should either add numbers or drop the claim. + ## Critical constraints - **Bootstrap**: never use `java.util.logging.*`, `java.nio.file.*`, or `javax.management.*` in `premain` code — see [docs/bootstrap_design_guidelines.md](docs/bootstrap_design_guidelines.md).