Add perf-review skill for pre-PR hot-path performance review#11912
Add perf-review skill for pre-PR hot-path performance review#11912dougqh wants to merge 1 commit into
Conversation
dougqh
added
type: documentation
tag: no release notes
|
🎯 Code Coverage (details) 🔗 Commit SHA: 218da4a | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
Adds a read-only `/perf-review` Claude Code skill under .claude/skills/perf-review/ that reviews a diff / branch / PR for tracer performance overhead using the perf rubric: 7 universal checks + the Java addendum (J1-J11) + ByteBuddy-Advice fix idioms, with a precision-first, advisory, verify-don't-verdict posture (do-no-harm / assume-hot). - SKILL.md: posture, workflow, PR-style output template, and a compact check index (pointers into the references). - references/guide.md: the narrative "how" (severity, hotness, categories). - references/checks.md: the confidence/severity cost-model + Java addendum. - references/example-review.md: a worked review to calibrate output. Also adds a "Review Guidelines" section to AGENTS.md that points at the skill for the pre-PR performance review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dougqh
force-pushed
the
dougqh/perf-review-skill
branch
from
050d46d to
218da4a
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 218da4a51f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| container kill. Flag aggressively. | ||
|
|
||
| **Raw unbounded cache.** No size or byte bound, keyed by data that grows with load (URL, SQL, | ||
| resource names). Fix: `DDCache.newFixedSizeWeightedCache(n, weigher, maxBytes)`. |
There was a problem hiding this comment.
This fix path names DDCache.newFixedSizeWeightedCache, but DDCache is only the cache interface; the static factory lives on datadog.trace.api.cache.DDCaches (internal-api/src/main/java/datadog/trace/api/cache/DDCaches.java). When /perf-review flags a SEV-1 unbounded-cache issue, following this guidance would produce an uncompilable recommendation or patch instead of the intended DDCaches.newFixedSizeWeightedCache(...) fix.
Useful? React with 👍 / 👎.
|
|
||
| **Mis-sized collections.** `ArrayList` grows 1.5×; `HashMap` doubles and rehashes. Both pay | ||
| allocation + copy on growth. Fix: pre-size accurately at construction. Note: `new HashMap<>(n)` | ||
| still rehashes at 75% fill — use `HashMap.newHashMap(n)` (JDK 19+) for accurate pre-sizing. |
There was a problem hiding this comment.
The skill is meant to review normal dd-trace-java production changes, whose shared Gradle config still targets Java 8 (gradle/java_no_deps.gradle), so this unconditional fix is unsafe outside explicitly newer source sets: HashMap.newHashMap(int) was added in JDK 19 and will not compile in most modules. A reviewer following this guidance on hot code would suggest an unusable API; prefer a Java 8-compatible capacity calculation/helper unless the target source set is known to be 19+.
Useful? React with 👍 / 👎.
|
|
||
| - **J7 — `substring`/slice → `SubSequence` zero-copy view** *(refines #1, #7)*. `String.substring`/`subSequence` allocates a fresh backing array per call. On a hot parse path (headers, tags, query strings, SQL/DBM, propagation) where the slice is **transient** — compared (`equals`/`startsWith`/`contains`/`indexOf`), parsed, or appended, then discarded — a `SubSequence` (offset+length view) is zero-copy and EA-elided **iff** the consumer takes a `CharSequence`/range (else the boundary `toString()` erases the win → add the overload or skip). **flag-as-measure** for the transient case (EA-dependent). The retention trap is **flag-with-confidence**: a `SubSequence` stored in a field/tag/collection/cache pins its *entire* backing String — a small window over a large string is a net memory loss — so a retained view must be materialized or `compact()`'d. Discriminator = transient (view, measure) vs retained (must detach). | ||
| - **J8 — Backtracking regex on external input → RE2J / bounded input** *(distinct from #2 compile-per-call)*. `java.util.regex` backtracks → exponential worst-case (ReDoS) on adversarial input — a CPU / tail-latency / DoS hazard, **not** an allocation one. Flag the **conjunction**: (a) input is user/external-controllable AND (b) the pattern is backtracking-prone (nested/overlapping quantifiers, `(a+)+`, unanchored `.*` around a quantified group). **flag-with-confidence** when both hold — SEV-2 (tail latency), **SEV-1** on a per-request AppSec/security-scan path (IAST Reporter / WAF run regex on untrusted input every request). Fix: RE2J (`com.google.re2j`, guaranteed linear; no backrefs/lookaround), anchor/de-nest, or hard-cap input length. | ||
| - **J9 — `Objects.hash(...)` varargs / boxing hash on a hot path → `HashingUtils`** *(refines #1)*. `Objects.hash(a, b, …)` is varargs — it allocates an `Object[]` per call and **boxes every primitive** arg; `Arrays.hashCode` and hand-rolled `31*h + …` combines that box also qualify. Per-span tag/key building, or the `hashCode()` of a hot value object, → a guaranteed per-call allocation + boxing. Fix: `datadog.trace.util.HashingUtils` — primitive `hash(long/int/boolean/char/…)` overloads (no boxing), `hash(Object,Object)` and `hash(int,int)` combiners (no array); for >2 fields fold pairwise through `hash(int,int)` (there is no varargs form, by design). flag-with-confidence (array + boxing are mechanism-determined) — SEV-2/3. |
There was a problem hiding this comment.
This rule groups Objects.hash with Arrays.hashCode and hand-rolled 31*h + … combines, then describes the whole set as guaranteed allocation/boxing. That is only true for varargs/boxing forms; a hot hashCode() using 31 * h + Long.hashCode(x) or an existing primitive array is allocation-free (and HashingUtils itself uses 31-based combines), so /perf-review would produce false-positive perf findings and recommend replacing already-correct code.
Useful? React with 👍 / 👎.
| - **`@Advice.AllArguments()` — deterministic lint.** Materializes a new `Object[]` boxing all arguments on every advised call; always escapes, EA cannot elide it. Fix: `@Advice.Argument(value=N)` for the one argument and type needed. | ||
| - **`@Advice.SkipOn(OnDefaultValue.class)` + cached boolean — the preferred feature-flag pattern.** Compute a `static final boolean` once, return it from `@Advice.OnMethodEnter`, suppress exit advice when disabled. Residual cost: one JIT-hoistable field read per call. Recommend this whenever a `Config.get()` shows up in advice. | ||
| - **`@Advice.Local` — prefer over `ThreadLocal`.** Carries per-invocation state from `OnMethodEnter` to `OnMethodExit` with no map lookup. | ||
| - **`@Advice.Origin` String allocation.** Injects class/method name as a `String` parameter — allocates per invocation at a non-static site. Prefer a `static final String` constant. |
There was a problem hiding this comment.
This flags every @Advice.Origin(...) String as a per-invocation allocation, but Byte Buddy injects origin strings as constants; the per-read recreation warning applies to reflective Method/Constructor origins, not string patterns. dd-trace-java has many hot advices intentionally passing #m strings, so this rule would create false positives and push reviewers toward static constants that cannot represent multi-method advice cleanly.
Useful? React with 👍 / 👎.
![datadog-prod-us1-6[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
This PR adds a read-only performance-review skill and documentation for pre-PR hot-path analysis. No tracer code changes. All cited utilities (HashingUtils, Strings, Hashtable, RE2J) are verified to exist; "coming" items are properly marked. One documentation inconsistency: example-review.md embeds reachability information as prose rather than the structured bullet-point format the template requires, which could confuse reviewers on output format consistency.
🤖 Datadog Autotest · Commit 218da4a · What is Autotest? · Any feedback? Reach out in #autotest
|
|
||
| ## Review Guidelines | ||
|
|
||
| Before marking a PR ready, run the applicable reviews below over the branch changes (the diff since the merge-base with `master`). These are **advisory, precision-first** checks — they surface high-severity issues early and are not merge gates. The linked guidelines are the tool-agnostic source of truth; in Claude Code each is also runnable as a `/`-skill. |
There was a problem hiding this comment.
I suspect we'll want to scrub the Claude-specific references, but I'll let others help me out here.
bm1549
left a comment
bm1549
left a comment
There was a problem hiding this comment.
Overall, this is a solid use of the progressive disclosure pattern and the AGENTS.md pointer to the skill file will likely get the Codex PR reviewer's attention, although it's not fully deterministic. A future iteration can use something like datadog-agent's agentic code review workflow, but that can be a follow-up task
Mostly left comments on the way the skill is structured, rather than the contents of the skill itself. Once these comments are resolved (and Codex's ones are either solved or dismissed), I think this is good to merge
|
|
||
| ## Review Guidelines | ||
|
|
||
| Before marking a PR ready, run the applicable reviews below over the branch changes (the diff since the merge-base with `master`). These are **advisory, precision-first** checks — they surface high-severity issues early and are not merge gates. The linked guidelines are the tool-agnostic source of truth; in Claude Code each is also runnable as a `/`-skill. |
There was a problem hiding this comment.
You can drop this - the agents won't need it
| Before marking a PR ready, run the applicable reviews below over the branch changes (the diff since the merge-base with `master`). These are **advisory, precision-first** checks — they surface high-severity issues early and are not merge gates. The linked guidelines are the tool-agnostic source of truth; in Claude Code each is also runnable as a `/`-skill. | |
| Before marking a PR ready, run the applicable reviews below over the branch changes (the diff since the merge-base with `master`). These are **advisory, precision-first** checks — they surface high-severity issues early and are not merge gates. The linked guidelines are the tool-agnostic source of truth. |
|
|
||
| ### Performance Review | ||
|
|
||
| Follow the performance-review guidelines in [.claude/skills/perf-review/SKILL.md](.claude/skills/perf-review/SKILL.md) — the do-no-harm / assume-hot posture, the hot-path checks (universal + Java J1–J11 + ByteBuddy-Advice idioms), and the confidence/severity model, with the detailed rubric in its `references/`. Scope: hot-path allocation, unbounded memory, repeated work, escaping objects, native-boundary crossings, and JVM-specific pitfalls. In Claude Code, run `/perf-review`. |
There was a problem hiding this comment.
I'd suggest moving the skill directory into .agents/skills/, as it's symlinked to .claude/skills/ anyways
Past that, you don't need to tell the agent that it's invocable as a skill - it can infer it from the frontmatter
| Follow the performance-review guidelines in [.claude/skills/perf-review/SKILL.md](.claude/skills/perf-review/SKILL.md) — the do-no-harm / assume-hot posture, the hot-path checks (universal + Java J1–J11 + ByteBuddy-Advice idioms), and the confidence/severity model, with the detailed rubric in its `references/`. Scope: hot-path allocation, unbounded memory, repeated work, escaping objects, native-boundary crossings, and JVM-specific pitfalls. In Claude Code, run `/perf-review`. | |
| Follow the performance-review guidelines in [.agents/skills/perf-review/SKILL.md](.agents/skills/perf-review/SKILL.md) — the do-no-harm / assume-hot posture, the hot-path checks (universal + Java J1–J11 + ByteBuddy-Advice idioms), and the confidence/severity model, with the detailed rubric in its `references/`. Scope: hot-path allocation, unbounded memory, repeated work, escaping objects, native-boundary crossings, and JVM-specific pitfalls. |
| rubric. Use whenever the user wants a performance / overhead / hot-path review, | ||
| asks to check a diff or PR for allocation / GC / memory / latency / startup cost, | ||
| mentions the "perf rubric" or the "do no harm / assume hot" tracer posture, or is | ||
| about to open a PR touching span lifecycle, tag maps, serialization/encoding, |
There was a problem hiding this comment.
about to open a PR
While I'm all for this, I'm not sure this is what you intended
| about to open a PR touching span lifecycle, tag maps, serialization/encoding, | ||
| decorators, propagation, or instrumentation — even if they just say "review this | ||
| for perf" without naming the rubric. Advisory and READ-ONLY: it reports ranked, | ||
| verify-first findings; it never blocks a merge and never edits code. |
There was a problem hiding this comment.
Don't need to say it doesn't block a merge to the agent
| verify-first findings; it never blocks a merge and never edits code. | |
| verify-first findings; it never edits code. |
| Review the current branch's changes for performance overhead in the dd-trace-java | ||
| tracer, using the tracer performance rubric bundled in `references/`. This is a | ||
| **low-friction advisory nudge**, not a gate: it reports findings and stops. It | ||
| never edits code and never blocks a merge. |
There was a problem hiding this comment.
| never edits code and never blocks a merge. | |
| never edits code. |
| span lifecycle (create / setTag / finish), tag-map ops, serialization/encoding, the | ||
| metrics/stats path, decorators, propagation (header read/write). | ||
|
|
||
| **Cold only with positive evidence:** one-time init, startup-only path, a genuinely |
There was a problem hiding this comment.
We don't necessarily need to grow the scope of this skill, but startup is something we should consider as "hot" too. No need to change anything since this skill won't regress startup, but something to keep in mind for the next performance skill
| - **Small, focused diff** (one method, a handful of files): report *every* genuinely | ||
| high-confidence finding, ranked by severity. A tight diff with four real allocation | ||
| smells should list all four (as the worked example does). | ||
| - **Large PR:** lead with the 1–2 highest-severity findings and note that lower-severity |
There was a problem hiding this comment.
One more can't hurt 😁
| - **Large PR:** lead with the 1–2 highest-severity findings and note that lower-severity | |
| - **Large PR:** lead with the 1–3 highest-severity findings and note that lower-severity |
| ## Output format | ||
|
|
||
| Follow this structure (see `references/example-review.md` for a full worked instance — | ||
| PR #11903, Bucket4j). Showing your suppressed lookalikes and what you cleared is not |
There was a problem hiding this comment.
Would not recommend having the agent pull a PR just for a format check
| PR #11903, Bucket4j). Showing your suppressed lookalikes and what you cleared is not | |
| ). Showing your suppressed lookalikes and what you cleared is not |
| Follow this structure (see `references/example-review.md` for a full worked instance — | ||
| PR #11903, Bucket4j). Showing your suppressed lookalikes and what you cleared is not | ||
| filler: it demonstrates the precision that makes the findings trustworthy. | ||
|
|
There was a problem hiding this comment.
I'm not positive if this will apply to the Codex reviewer or not, but it can't hurt to try
| When providing suggestions as code review comments, prefix the comments with "perf: " |
2 participants
What
Adds a read-only
/perf-reviewClaude Code skill (.claude/skills/perf-review/) that reviews a diff / branch / PR for tracer performance overhead using the performance rubric, and notes the pre-PR performance review underAGENTS.md's PR conventions (alongside/techdebt).The skill flags hot-path allocation, unbounded memory, repeated work, escaping objects, native-boundary crossings, and JVM-specific pitfalls (escape analysis, JNI / virtual-thread pinning, backtracking-regex ReDoS, varargs/boxing hashing,
String.format, ByteBuddy-Advice anti-patterns).Posture: do-no-harm / assume-hot, advisory (never blocks a merge), precision over recall (silent when unsure), and verify-don't-verdict (every finding routes to Benchmark → Profile → Improve → Guard). It is read-only — it reports ranked findings and never edits code.
Layout
SKILL.md— posture, workflow, a PR-style output template, and a compact check index.references/guide.md— the narrative "how": severity model, hotness rubric, categories with examples.references/checks.md— the confidence/severity cost-model: 7 universal checks + Java addendum (J1–J11) + ByteBuddy-Advice fix idioms.references/example-review.md— a worked review to calibrate output.Usage
Run
/perf-reviewbefore opening a PR. It computes the branch diff against the upstream merge-base (or reviews specific files you point it at) and reports findings with confidence, severity, the reachability path, and a verify-with suggestion.Validation
Back-tested against a curated perf-review corpus of merged/closed PRs with known expected verdicts — reproduced the expected precision (false-positive traps stay silent), recall (real regressions flagged), and nuance (severity-regime demotion, resolve-via-sink, correctness-lens routing).
Notes
AGENTS.mdline.tag: no release notes.🤖 Generated with Claude Code