diff --git a/.claude/skills/perf-review/SKILL.md b/.claude/skills/perf-review/SKILL.md new file mode 100644 index 00000000000..5f6ab16c169 --- /dev/null +++ b/.claude/skills/perf-review/SKILL.md @@ -0,0 +1,246 @@ +--- +name: perf-review +description: >- + Performance-overhead review of a code diff / branch / PR for the dd-trace-java + tracer. 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) using the tracer performance + 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, + 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. +user-invocable: true +context: fork +allowed-tools: + - Bash + - Read + - Grep + - Glob +--- + +# Performance Review + +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. + +## Why this exists (read first — it sets the whole posture) + +The tracer shares the customer's process, heap, and latency budget. **Do no harm**: +overhead is a form of incorrect behavior that can escalate to real customer harm — +missed SLAs, OOM kills, container restarts, cold-start churn. So the review's job is +to catch overhead the customer would feel, and to do it *without becoming noise*. + +Two forces are in tension, and the resolution defines everything below: + +- **Assume hot.** We don't know what's on a customer's critical path. Absent positive + evidence of cold, assume the code runs on every request, under load, at full + concurrency. The burden of proof runs toward *cold*: ask "is there evidence this is + cold or guarded?" — not "is there evidence this is hot?" (that rationalizes itself + into "probably not"). +- **Precision over recall — be silent when unsure.** A false-positive-prone review + dies of being ignored. Over-flagging kills it faster than under-flagging. This + actively fights your default to be comprehensive and helpful: here, *not* flagging + a borderline case is the correct, skilled move — not a miss. + +You reconcile them with the **confidence axis** and **verify-don't-verdict** (below): +assume-hot makes you *look* everywhere; precision makes you *speak* only when the +mechanism is certain or the severity is catastrophic. + +## Core rules + +- **Findings are prompts to *verify*, not verdicts.** You reason statically; you + cannot render a performance verdict from a code read. Every finding routes into + **Benchmark → Profile → Improve → Guard**. Phrase each as *"this looks like X; + verify with Y"* — never "this is slow." +- **Confidence axis on every finding:** + - **flag-with-confidence** — the cost is *mechanism-determined* and visible in the + code: allocation, boxing, copying, unbounded growth, a native crossing. State it + plainly. + - **flag-as-measure** — the cost depends on JIT/GC/optimizer decisions you can't + see from source: escape elision, inlining/devirtualization, GC impact. Phrase as + "may X; verify with a profiler/benchmark," never as a certainty. +- **Predicate-with-default, not a banned-API list.** Don't flag "you called + `String.format`." Flag *"an eager, unconditional expensive call on a hot, + instrumentation-reachable path."* The same API is fine on a cold path. Two failure + shapes, different fixes: result usually **discarded** → gate/defer; result always + **needed but costly** → cheapen/cache. +- **Resolve interprocedurally — this is the review's whole reason to exist.** A + peephole lint can't answer "reachable from a hot entry, unconditional along the + way." Trace *up* (who calls this? is it reachable from an `@Advice` root / per-span + callback / request handler?) and *down* (follow callbacks, hooks, and listeners to + their **sink** before flagging). If a per-span hook's every reachable sink is an + atomic counter (`LongAdder`, `AtomicLong`) or a no-op-when-disabled, stay silent — a + "verify contention" nudge there is noise. +- **Make the reachability path the headline.** The reachability claim is the most + valuable *and* least reliable part of a finding — residual false positives cluster + in "called it unconditional, missed an upstream guard." Say *"reachable from + `Foo.onEnter` via A→B→C, no guard on that path"* so the reader can check the + shakiest link at a glance. +- **Only flag toward a fix that exists.** A finding must be actionable *now*. Route to + a mechanism that has landed (see the toolkit note in `checks.md` — cite only what + exists; name "coming" primitives as coming). Don't flag a pattern whose only fix is + a mechanism that isn't built yet. +- **Triage by severity.** Flag SEV-1 (unbounded memory / OOM, cardinality blowups) + *aggressively* — a false positive there is cheap insurance against a container kill. + Flag low-severity CPU-micro *conservatively or not at all* — false positives there + only erode trust. +- **Never flag the *absence* of a cache on high-cardinality input.** For open-cardinality + data (raw SQL with literals, per-request strings), *not* caching is the correct + choice — caching it would be the worse SEV-1. Flag a cache *keyed by* high-cardinality + data; never flag the decision not to cache. +- **A *visibly contestable* perf tradeoff shipped without data → one soft flag-as-measure.** + The trigger is narrow: the change makes a **visible tradeoff that could itself regress** — + it removes a lock / guard / synchronization, swaps in a hand-rolled cache or data + structure, or explicitly claims "faster / optimized" — **and** ships no benchmark or + profile. There a static read genuinely can't tell a win from a regression, so raise one + soft *flag-as-measure* nudge: *"this trades ; verify with a JMH benchmark / JFR."* + Do **not** fire it otherwise — if nothing in the diff could plausibly regress, there is + nothing to measure, so stay silent. Specifically not for: a **mechanically-obvious win** + (hoisting an invariant out of a loop, a denser data structure, removing an allocation); + **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. + +## Workflow + +### Step 1 — Get the code to review + +**If the user points you at specific files or pasted code** ("review this class / this +method for perf"), review those directly — skip the diff and go to Step 2 with the same +hot-path mapping and checks. + +**Otherwise, review the branch changes.** Find the merge-base against the DataDog +upstream `master` and diff against it: + +```bash +UPSTREAM=$(git remote -v | grep -E 'DataDog/[^/]+(\.git)?\s' | head -1 | awk '{print $1}') +[ -z "$UPSTREAM" ] && UPSTREAM="origin" +MERGE_BASE=$(git merge-base HEAD ${UPSTREAM}/master) +echo "Reviewing changes since $MERGE_BASE" +git diff $MERGE_BASE --stat +git diff $MERGE_BASE --name-status +``` + +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. + +### Step 2 — Map the changed code onto hot paths + +For each changed method, decide *which multiplier applies* before flagging anything. + +**Hot anchors** (reachable ⇒ assume hot): `@Advice.OnMethodEnter`/`OnMethodExit`, +per-span / per-trace callbacks, request / message handlers, streaming chunk handlers. +**Hot-path map** (where cost is multiplied per-span × spans/request × requests/sec): +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 +rare error branch, or behind a guard that provably fires rarely. Watch the +**interprocedural trap** — a method three helpers deep from an `@Advice` entry is +still hot. And note **domain adjustment**: large-denominator domains (LLMObs, CI +Visibility, DSM) absorb per-call CPU/alloc cost, but the risk *inverts* to payload +memory (SEV-1); streaming handlers fire per-chunk, so the large-denominator relief +suspends inside them. See `guide.md` §6. + +### Step 3 — Apply the checks + +Run the changed hot-path code against the rubric. Keep the check index below in mind; +open the references for the precise conditions, confidence, severity, and fix: + +- **`references/guide.md`** — the narrative "how": severity model, hotness rubric, + the 6 categories with worked examples, and the false-positive traps. Read this first + if you're calibrating judgment. +- **`references/checks.md`** — the precise cost-model: 7 universal checks + the Java + addendum (J1–J11) + the ByteBuddy-Advice fix idioms + the toolkit-availability note. + Read this for the exact confidence/severity/fix of a specific pattern. + +### Step 4 — Resolve, then emit + +Before writing a finding: confirm the reachability path, confirm it's unconditional +along that path (check for upstream guards), and follow any hook/callback to its sink. +Drop anything that resolves to benign. Then report in the format below. + +**How many findings to report — scale with diff size:** +- **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 + ones may exist — don't bury the important one under a wall of CPU-micro nits. +- Either way, the gate is *confidence*, not a count: silence on the uncertain ones is + what earns the review its credibility. + +## 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 +filler: it demonstrates the precision that makes the findings trustworthy. + +```markdown +# Perf Review — + +**Scope reviewed:** + +## Confirmed findings + +### 1. + +- **Confidence:** flag-with-confidence | flag-as-measure +- **Reachability:** +- **Rubric check:** <#N / JN> +- **Severity:** SEV- +- **Fix / verify-with:** + +## Correctly suppressed (not flagged) + + +## Checked, no issue + + +## Summary + +``` + +If nothing survives the confidence bar, say so plainly — "No high-confidence hot-path +findings; here's what I checked and cleared." A clean review is a valid, valuable +result, not a failure to find something. + +## Check index (the map — details in the references) + +**Universal (language-agnostic):** +1. Per-span/per-call allocation on a hot path (retained/escaping) — SEV-2/3 +2. Repeat work across calls (regex compile / format / parse / concat recomputed) — SEV-2/3 +3. Unbounded memory / collection, or keyed by high-cardinality input — **SEV-1** +4. Expensive work on the critical path that could be deferred — SEV-1/2 +5. Polymorphic dispatch defeating inlining/devirt — flag-as-measure — SEV-2/3 +6. FFI / native-boundary crossing per-item (not batched) — SEV-1/2 +7. Escape / allocation-elision defeated by a refactor — flag-as-measure — SEV-2/3 + +**Java addendum (JVM-specific — full text + mechanism in `checks.md`):** +- **J1** escaping allocation defeats Escape Analysis · **J3** JNI crossing + virtual-thread + pinning · **J4** GC pressure → tail latency · **J5** cardinality-sensitive aggregator + (**SEV-1**) · **J6** `WeakReference.get()` in a probe loop strengthens the ref · + **J7** `substring` → `SubSequence` zero-copy view · **J8** backtracking regex on + external input → RE2J (ReDoS) · **J9** `Objects.hash(...)` varargs/boxing → + `HashingUtils` · **J10** hot-path `String.format` → `Strings` · **J11** composite-key + maps → `Hashtable`. +- **J2** megamorphic dispatch is **PARKED** — do *not* raise megamorphism findings in + review yet (kept as author reference only; it needs a standing audit, not per-PR + flagging). See `checks.md` for why. +- ByteBuddy-Advice idioms (`Config.get()` hoisting, `@Advice.AllArguments` → + `@Advice.Argument`, `@Advice.SkipOn`+cached-boolean, `@Advice.Local`, `switch(String)` + three-tier) — in `checks.md`. + +J7–J11 route an *existing* #1/#2/#3 finding to a landed reusable fix — they are not new +triggers. Don't raise a finding you wouldn't have raised anyway. diff --git a/.claude/skills/perf-review/references/checks.md b/.claude/skills/perf-review/references/checks.md new file mode 100644 index 00000000000..2d2a911f361 --- /dev/null +++ b/.claude/skills/perf-review/references/checks.md @@ -0,0 +1,65 @@ +# Performance Review Checks — AI-review cost model + +Operationalizes the narrative **Performance Review Guide** (`guide.md`) into diff-applicable checks for AI PR review. Complements benchmark-based regression-blocking: benchmarks catch *measured* regressions on *covered* ops; this catches *un-benchmarked* pattern-smells in *any* diff. Grounded in the guide's **Tracer Principles** (do-no-harm, assume-hot) and its severity model. Read this alongside `guide.md` — the guide is the narrative "how"; this is the precise confidence/severity cost-model. + +## Posture (read first) +- **Advisory, not blocking** initially. A nondeterministic, false-positive-prone check that blocks merges dies of being ignored (false positives) or gives false confidence (false negatives). Earn blocking only after precision is proven, and likely only on the deterministic-lint subset. +- **Precision over recall — silent when unsure.** Only flag high-confidence issues on real hot paths. Over-flagging kills the check. (This fights the model's default to be comprehensive/helpful — state it explicitly in the prompt.) +- **Resolve-via-sink before flagging callbacks and hooks.** A per-span callback or scope hook registration *looks* like a hot-path cost but may be safe once you follow into the registered listener. If every reachable sink is an atomic counter (`LongAdder`, `AtomicLong`) or no-op-when-disabled, stay silent — a "verify contention" nudge there is noise. Follow the full listener chain before emitting a finding. +- **Findings are prompts to *verify*, not verdicts.** The AI reasons statically — by the doc's own "Don't Assume; Measure," it *cannot* render a perf verdict. Every finding routes into Benchmark → Profile → Improve → Guard. Phrase as "this looks like X; verify with Y." +- **Triage by severity** (from the doc): flag SEV-1 (memory/OOM) patterns *aggressively* (a false positive is worth catching a container-kill); flag low-severity CPU-micro *conservatively or not at all* (false positives there only erode trust). +- **Scope**: the diff × known hot paths. Don't review the world. +- **Confidence axis** on every finding: **flag-with-confidence** (mechanism-determined: allocation, dispatch, copying, crossing, unboundedness) vs **flag-as-measure** (opaque: depends on JIT/GC/optimizer decisions — escape elision, inlining, GC impact). +- **Visibly-contestable perf tradeoff without data → soft flag-as-measure.** Narrow trigger: the change makes a **visible tradeoff that could itself regress** — removes a lock/guard/synchronization, swaps in a hand-rolled cache/structure, or explicitly claims "faster/optimized" — **and** ships no benchmark/profile. Then a static read can't tell win from regression → one soft flag-as-measure nudge ("trades X for Y — verify with a JMH benchmark / JFR"). If nothing in the diff could plausibly regress, there's nothing to measure → stay silent. Explicitly NOT fired for: a mechanically-obvious win (hoist-invariant, denser structure, removed allocation); routine adoption of a known-better idiom (lower-overhead builder/API/toolkit primitive, no visible downside); or a change that ships a benchmark/JFR (well-evidenced). One line, a nudge not a code-pattern finding. + +## Hot-path map (where cost matters — and the multiplier) +Span lifecycle (create / setTag / finish), tag map ops, serialization/encoding, the metrics/stats path, decorators, propagation (header read/write). **Multiplier: per-span × spans/request × requests/sec.** A per-span cost is multiplied massively; a per-process/once cost is negligible. The AI must reason about *which multiplier applies* before flagging. + +## Universal checks (language-agnostic) +Format: **pattern** — *expensive when (the interprocedural condition to trace)* — confidence — severity — fix. + +1. **Per-span/per-call allocation on a hot path** — *per-span (or hotter) AND the object isn't trivially short-lived (it's retained, returned, captured, or passed across a boundary)* — flag-with-confidence if clearly per-span + retained; flag-as-measure if lifetime/escape is borderline — SEV-2/3 (→SEV-1 if unbounded) — fix: reuse, pool, dense/positional storage, defer out of the hot path. +2. **Repeat work across calls/traces** — *string concat / case-conversion / regex *compile* / format / parse recomputed each hot-path call, on a recurring (low-cardinality) input or allocating each time* — flag-with-confidence — SEV-2/3 — fix: memoize (bounded — see #3) or compile-once (hoist regex to static). +3. **Unbounded memory / collection** — *a cache/map/collection with no size+byte bound, or keyed by a high-cardinality input (per-request data, raw strings)* — flag-with-confidence (unboundedness is structurally visible) — **SEV-1 (OOM / container-kill — top severity)** — fix: bound by count *and* bytes; or don't cache high-cardinality inputs (opt out). +4. **Expensive work on the critical path that could be deferred** — *heavy compute / parse / normalize / serialize / I/O / lock on the synchronous request or span-finish path, that could be moved* — flag-as-*consider* (deferability is contextual) — SEV-1/2 — fix: defer to background/writer thread, lazy-compute, batch. +5. **Polymorphic dispatch on a hot path** — *a hot call site becomes polymorphic enough to defeat the runtime's inlining/devirtualization (real for JIT runtimes — JVM/.NET/V8; AOT/interpreted differ)* — **flag-as-measure** ("may defeat devirtualization; verify on the target runtime") — SEV-2/3 — fix: keep hot call sites mono/bi-morphic; specialize. +6. **FFI / native-boundary crossing on a hot path** *(central to the shared-core effort)* — *a native crossing per-span/per-item (not batched), or transporting strings/objects rather than primitives/IDs* — flag-with-confidence (boundary cost is mechanism-determined; runtime-specific pinning → addendum) — SEV-1/2 (SEV-1 if it blocks/pins under concurrency) — fix: batch (one per flush, not per item); transport interned IDs not strings; keep crossings off the hot/concurrency path. +7. **Escape / allocation-elision defeated** *(Java/Go/.NET/V8 all have a version)* — *a refactor makes a previously-local object escape (stored, returned, captured by a closure, passed to a virtual/non-inlined call) → silent heap allocation on a hot path* — **flag-as-measure** ("may now escape and allocate; verify with an allocation profiler") — SEV-2/3 — fix: keep it local; avoid the escaping store/capture. + +## Deterministic-lint candidates (DON'T spend AI budget — make these real lints) +Fixed-signature, mechanically checkable: +- per-call cache / regex / expensive-object creation that should be static/once +- boxing in specific hot APIs +- using a string-API where an id-API exists on a hot decorator +- the existing convention rules (e.g. don't extract one-shot instrumentation methods to constants) +- *(grows as patterns prove mechanically checkable — migrate them off the AI as they stabilize)* + +## Java addendum (JVM-specific — mechanism authored with JIT-developer authority; **calibrate production-priority against your own escalation history**) +Refines the universal checks with JVM mechanics. Quarantined here, for the Java audience that has the substrate. + +**Scope (2026-07-08):** the primary optimization target is **C2 / Java 11+ (HotSpot)** — the mechanisms below are stated in those terms (inline-cache/`TypeProfileWidth` model, C2 speculative inlining, EA). C1-only, OpenJ9/J9, GraalVM, and Java 8 should still benefit but are the minority case we don't *tune* for. Pairs with the benchmark JVM standardization (Java 17 HotSpot). + +- **J1 — Escaping allocation defeats Escape Analysis** *(refines #1, #7)*. The JVM scalar-replaces only *non-escaping* short-lived objects. An object stored in the tag map / span / a collection, iterated at serialization, or passed to a virtual/megamorphic call **escapes** → EA can't elide it → real heap allocation. The trap: *"the JIT will scalar-replace it" is false for escaping objects* — the dense-store −48% came from removing exactly the escaping per-tag wrappers; an earlier no-Entry change measured ~0 because *those* entries were transient/EA-eligible. **Verify in JFR — EA'd objects don't appear in alloc profiles, so a surviving `Entry` in the profile *proves* it escapes.** +- **J2 — Megamorphic dispatch** *(refines #5)*. **Status — PARKED for PR-review flagging (2026-07-08): do NOT raise megamorphism findings in review yet.** Kept as author-reference + the standing-audit target described below, not as an active review idiom. Rationale: it's too in-the-weeds to land with most devs, and the rubric must first bank *legible* wins (allocation, unbounded memory, regex — what anyone can see in a profiler) to earn trust before deploying the subtle JIT checks. Revisit once the rubric has a track record. (Mechanism below stands; it's the flagging that's held.) A hot call site seeing **≥3 receiver types with no dominant one** goes megamorphic. **Do not frame this as "the virtual call is slow" — a well-predicted indirect branch is a couple of cycles; the dispatch is a rounding error.** The cost is the **optimization fence the un-inlinable call erects on *both* sides**: caller-side, the args escape into an opaque callee → no scalar-replacement/EA on them, no constant-propagation *into* the call, caller-saved registers spilled across it, no hoist/reorder across the boundary; callee-side, it's never specialized to *this* caller, so the arg types/constants that would have collapsed its internal branches and devirtualized its *own* downstream calls stay invisible. Inlining is what lets the two bodies optimize as **one unit**; the megamorphic call severs that — on the hottest paths that is the whole bill. **C2 rescue conditions (the primary target):** ≤2 types stays **bimorphic** (inlinable); a **dominant receiver** (≥`TypeProfileMajorReceiverPercent`, default 90%) still gets guarded mono-inline + uncommon-trap fallback, so a *skewed* site is usually fine — but watch **bimodal oscillation** (a recurring rare type → deopt thrash). The danger zone is the **flat ≥3 distribution** (`TypeProfileWidth`=2 → profile overflow → itable v-call, no inline). Fixes, cheapest first: keep the site to ≤2 types; **gate-when-empty** so the common case skips the fan-out (a default-empty listener array); **collapse N impls into one** `final` class with an internal state/size-class switch (e.g. retire a legacy map impl so the optimized one is the sole impl → every `TagMap` site monomorphic); or **call-site-split/unroll** a stable-order fan-out (catalog #7). **flag-as-measure** — opaque; whether it bites depends on which impls actually load + the runtime receiver mix. Confirm with `-XX:+PrintInlining` (`not inlined (megamorphic)`) / JITWatch, not a code read. **Diff-review blind spot → needs a standing audit.** The worst megamorphic sites *accumulate*: no single PR introduces them (each only nudges the type count by one), so a per-PR check catches only a PR that *widens* a site — the pre-existing hazards are invisible to it. The heaviest is `Context.get`/`with` dispatching across `Empty`/`Singleton`/`Indexed`(+wrapper) impls — the hottest path in the system (catalog #11). These want a **periodic PrintInlining census of the known hot sites**, run independent of any PR, not diff review. +- **J3 — JNI / native crossing: overhead + virtual-thread pinning** *(refines #6)*. JNI call ≈ 100ns–1µs (state transition, arg pin/copy, no inlining across); string args via `GetStringUTFChars` = UTF-16→UTF-8 copy. **A JNI call from a virtual thread pins the carrier** → no other vthreads on that carrier run while pinned → concurrency collapse for vthread-reliant apps. Fix: batch at flush on the **writer (platform) thread**, keep the app-vthread path pure-Java, transport interned IDs not strings; `@CriticalNative` only for short primitive ops (no `JNIEnv`, no object args, no GC-safe state — severe constraints). Flag the crossing + pinning risk (mechanism); overhead magnitude → measure. +- **J4 — GC pressure → *tail* latency** *(refines #1)*. Hot-path allocation → more GC → STW pauses → app **tail** latency, not just throughput (the tracer shares the app heap). ZGC has short pauses but isn't common — assume G1/Parallel. flag-as-measure ("may raise tail latency; verify under load at a realistic heap"). +- **J6 — Reference strengthening in weak-cache scans** *(refines #1, #2)*. Calling `WeakReference.get()` (or `SoftReference.get()`) inside a cache-probe loop to identify the referent **strengthens** the reference — the returned strong ref keeps the object alive until it goes out of scope, defeating the purpose of the weak reference. Pattern to flag: a loop over a weak-ref cache that calls `.get()` for identity/equality comparison on every slot probed. Fix: store a stable key (e.g. `System.identityHashCode(context)`) in the wrapper at construction time; compare the key first (plain int, no strengthening); call `.get()` only on a key match (the right moment — you're about to use the referent anyway) or to detect eviction (`get() == null`). **flag-with-confidence** when `.get()` appears inside a probe loop on a hot path — SEV-2/3. + +- **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. +- **J10 — hot-path `String.format` / string munging → `Strings` (+ `SubSequence`)** *(refines #2)*. `String.format` parses the format string, boxes its args, and allocates on every call — never on a hot path; hand-rolled case-conversion, class/resource-name munging, blank-checks, and truncation recomputed per call qualify too. Fix: `datadog.trace.util.Strings` — allocation-aware `replace`/`truncate(CharSequence)`/`isBlank`/`getResourceName`/`getClassName`/…; for **transient substring compares** prefer a `SubSequence` view (J7); for plain assembly, direct concatenation beats `format`. flag-with-confidence for `String.format` on a hot path; flag-as-measure for borderline munging — SEV-2/3. +- **J11 — composite / multi-dimensional key maps on a hot path → `Hashtable` / `ConcurrentHashtable`** *(refines #1, #3)*. `Map>` nesting, or a `HashMap` keyed by a composite key (client-side stats, per-`(service, operation, …)` aggregation), allocates nested maps + `Entry` objects + boxes keys on the hot aggregation path. Fix: `datadog.trace.util.Hashtable` (single-threaded, composite-key D1/D2 tables — landed) or `datadog.trace.util.ConcurrentHashtable` (lock-free concurrent, **coming**) — positional composite keys, fewer allocations. flag-as-measure — SEV-2/3. +- **Toolkit availability — cite only what exists.** Available today: `Strings`, `SubSequence`, `HashingUtils`, `Hashtable` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`). Coming (name as "coming", don't imply it's present): `ConcurrentHashtable`, `StringIndex` (immutable string set/map), `UTF8BytesString.Cache` (recurring-string interner), wider `IntegerCache` (http-status/port boxing), `DDCache` inlining. **J7–J11 route an *existing* #1/#2/#3 finding to a reusable fix — they are not new flag-triggers. Don't raise a finding you wouldn't have raised anyway; the posture (precision, silent-when-unsure, findings-cap-scales-with-diff — see `SKILL.md`) is unchanged.** +- **J5 — Cardinality-sensitive aggregator** *(domain-specialized #3)*. Some structures are invisible to the generic "unbounded collection" check because the risk is *cardinality*, not raw size: a config- or user-driven value (tag key, resource name, HTTP URL) feeding a **cardinality-sensitive aggregator** (e.g. the conflating metrics aggregator — each unique label combination = one aggregate; a `maxAggregates` cap bounds OOM but high-cardinality input *thrashes* it: constant eviction, garbled metrics). flag-with-confidence when config/user-driven values reach an aggregator with a per-key budget — **SEV-1** (same class as unbounded memory: correctness + heap impact). Fix: bound the source cardinality before it enters the aggregator, or use sentinel substitution for over-cap values. This surfaced on merged production code more than once in back-test calibration — the capstone pattern where the bot's value concentrates. + +## Instrumentation (ByteBuddy Advice) idioms — dd-trace-java-specific fixes +The core rule is a **predicate-with-default, not a banned-API list**: don't flag "you called `String.format`"; flag *"an eager, unconditional expensive call on an instrumentation-reachable path."* Two failure shapes, different fixes: (1) the result is usually **discarded** → **gate/defer** (parameterized logging, `isEnabled()` guard) — the cost is avoidable; (2) the result is always **needed but costly** → **cheapen/cache** — gating does nothing. The discriminator (eager? unconditional? guarded? result-needed?) is interprocedural — that's the review's job (§ workflow). These idioms are the actionable fixes when a universal/Java finding lands on an `@Advice` path: + +- **`Config.get()` / `InstrumenterConfig.get()` on a hot path — flag-with-confidence.** Walks a config-resolution chain; not a free read. Fix: hoist to a `static final` field, or compute once in the constructor. Common trap: the call is buried in a helper invisible at the advice site — grep transitively. (The single most recurring finding in calibration — five independent occurrences.) +- **`@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. +- **Java Stream API on an advice/hot path — flag-with-confidence.** `stream()`/`IntStream` allocate `Spliterator` + pipeline objects per call; JIT elision is fragile. Fix: a plain `for` loop (`cstyleFor`/`enhancedFor`/`forEach`/`iterator` are all on par, ≈0 alloc). See guide §2 for the benchmark numbers and the three silencing exceptions (cache-miss body, length-guarded error path, cold/startup). +- **`switch(String)` — three-tier fix ranking.** (1) resolve to a constant `long` id (folds on any JIT); (2) open-addressed table (never folds but always inlinable, low-variance); (3) plain `switch` (fine for small, inlinable dispatch). Flag *large* switches (inline-budget exhaustion is mechanism-certain); flag *small* switches on hot constant-arg paths only as a version-conditional soft-alert. diff --git a/.claude/skills/perf-review/references/example-review.md b/.claude/skills/perf-review/references/example-review.md new file mode 100644 index 00000000000..de17100768c --- /dev/null +++ b/.claude/skills/perf-review/references/example-review.md @@ -0,0 +1,59 @@ +# Perf Review — PR #11903 (Bucket4j instrumentation, demo) + +**PR:** https://github.com/DataDog/dd-trace-java/pull/11903 +**Rubric:** `checks.md` + `guide.md` (this skill's references) +**Scope reviewed:** `Bucket4jDecorator.onConsume` — runs on every `Bucket#tryConsume` call (tracing hot path; multiplier = per-call × calls/sec). +**Method:** Diff reviewed independently of the PR description (description ignored per request). + +## Confirmed findings + +### 1. Per-call config lookup + Set allocation +```java +InstrumenterConfig.get().isIntegrationEnabled(singleton("bucket4j-tier"), true) +``` +`Collections.singleton(...)` allocates a new `SingletonSet` every call, plus a config lookup, for a value that doesn't change per-call. +- **Confidence:** flag-with-confidence +- **Rubric check:** #2 (repeat work on invariant input) +- **Severity:** SEV-2/3 +- **Fix:** hoist to a `static final boolean` (or cache in a field) computed once; eliminate the per-call allocation. + +### 2. `Arrays.stream(...).filter(...).findFirst()` in the hot path +Builds a Stream pipeline (Stream + Spliterator + pipeline stages + captured lambda) every call just to find the first threshold ≥ tokens. +- **Confidence:** flag-with-confidence +- **Rubric check:** #1 / #5 (per-call allocation + unnecessary indirection) +- **Severity:** SEV-3 +- **Fix:** plain `for` loop over `TIER_THRESHOLDS`, no Stream. + +### 3. `Objects.hash(bucket, tokens, consumed)` in `onConsume` +Varargs `Object[]` allocation + boxing of `tokens` (long) and `consumed` (boolean) on every call. Runs unconditionally, not gated behind the tier flag. +- **Confidence:** flag-with-confidence +- **Rubric check:** J9 +- **Severity:** SEV-2/3 +- **Fix:** `datadog.trace.util.HashingUtils` (no boxing, no array). + +### 4. Eager string concatenation in `LOGGER.debug(...)` +```java +LOGGER.debug("bucket4j tryConsume tokens=" + tokens + " consumed=" + consumed + " bucket=" + bucket); +``` +Builds the string (StringBuilder + `bucket.toString()`) unconditionally, even when debug logging is disabled. +- **Confidence:** flag-with-confidence +- **Rubric check:** #2 / J10 +- **Severity:** SEV-2/3 +- **Fix:** SLF4J parameterized form `LOGGER.debug("bucket4j tryConsume tokens={} consumed={} bucket={}", tokens, consumed, bucket)`, or guard with `isDebugEnabled()`. + +## Correctly suppressed (not flagged) + +`private static final int DEFAULT_LIMIT_KEY = Objects.hash("default", 100L);` + +Textually the same `Objects.hash` pattern as finding #3, but this one runs once at class-init (cold path), not per-call. Per the rubric's precision-over-recall posture (silent when unsure / don't erode trust with lookalike false positives), this is correctly **not** flagged. + +## Checked, no issue + +- No unbounded memory / cardinality-sensitive aggregator (check #3, J5) — nothing cached. +- No FFI/native-boundary crossing (check #6, J3). +- No megamorphic-dispatch finding raised — J2 is explicitly parked in the rubric, not an active review idiom. +- String-literal tag keys (`"bucket4j.tier"`, etc.) — JVM interns literals automatically, no per-call allocation cost. + +## Summary + +4 confirmed hot-path findings, all SEV-2/3 (allocation/CPU — none unbounded or OOM-adjacent). 1 lookalike correctly suppressed as cold-path. diff --git a/.claude/skills/perf-review/references/guide.md b/.claude/skills/perf-review/references/guide.md new file mode 100644 index 00000000000..23d450bf143 --- /dev/null +++ b/.claude/skills/perf-review/references/guide.md @@ -0,0 +1,276 @@ +# Java Tracer Performance Review Guide + +--- + +## Tracer Principles + +Two lines establish everything that follows. + +**Do no harm.** The tracer shares the customer's process, heap, and latency budget. Harm is +ordered by severity: crashes first, then security, then incorrect behavior, then adverse +performance. Performance overhead is a form of incorrect behavior — a non-directly-observable +side effect that can rise to directly observable customer harm: missed SLAs, OOM kills, container +restarts, cold-start churn. + +**Assume hot.** We don't know a priori what will be on the critical path in a customer's +application. In the absence of evidence, assume the code runs on every request, under load, at +full concurrency. There are exceptions — schedulers, startup code, I/O-heavy paths — but the +default is: *assume hot unless there is positive evidence of cold*. + +**Advisory, not blocking.** The rubric is a low-friction nudge alongside the developer's path — +not a wall across it. Flag the 1–2 highest-severity findings per PR. Stay silent when unsure. +Over-flagging kills the check faster than under-flagging. + +--- + +## Severity Guidelines + +| Severity | Type | Usual Cause | +|---|---|---| +| **SEV-1** | OOM / container kill | Unbounded memory growth | +| **SEV-1/2** | Response time — median | Expensive work on the critical path | +| **SEV-1/2** | Response time — tail latency | Allocation rate → GC pauses (shared heap) | +| **SEV-2** | Startup latency | Eager class loading, init, transformation | +| **SEV-2/3** | CPU overhead | General tracer activity, background work | + +CPU overhead alone is the lowest priority — it's a cost issue, not a correctness one, and +escalates only when it causes latency. + +**The denominator matters.** Severity is cost relative to the instrumented operation. A 2 µs tag +op on a sub-millisecond HTTP span is a large fraction of the operation. The same 2 µs on a 500 ms +LLM call is negligible. Large-denominator domains (LLMObs, CI Visibility, DSM) get lower +CPU/alloc severity — but the risk *inverts*: payload memory (large prompts, job metadata, +accumulated output) becomes SEV-1. + +**Default-state changes multiply severity.** A one-line `DEFAULT_X_ENABLED = true` flip applies +the enabled-path cost to every user. Scrutinize heavily regardless of diff size. + +--- + +## Hotness Rubric + +The key question when reviewing any code: *is this on a hot path?* + +The default answer is yes. The burden of proof runs toward cold. Ask "is there evidence this is +cold or guarded?" — not "is there evidence this is hot?" (that rationalizes itself into "probably +not"). + +**Hot anchors.** Paths are hot when reachable from: +- `@Advice.OnMethodEnter` / `@Advice.OnMethodExit` (per-advised-call) +- Per-span or per-trace callbacks +- Request or message handlers +- Streaming chunk handlers (even if the overall stream is slow — see §6) + +**Cold only with positive evidence.** A path is cold if it is: a one-time init, a startup-only +path, a genuinely rare error branch, or behind a guard that provably fires rarely. + +**Watch for the interprocedural trap.** Hot entry points are often indirect. A method buried +three helpers deep from an `@Advice` entry is still hot. Trace up before assuming cold. + +--- + +## Categories of Issues + +In approximate order of severity and frequency: + +1. **Unbounded Memory** — collections or aggregators that grow without a bound +2. **Repeated Allocation on Hot Paths** — regex compile, format strings, streams per call +3. **Per-span Escaping Allocation** — wrapper objects, defensive copies, capturing lambdas +4. **Wrong Collection Type** — heavier type than needed, missing pre-sizing +5. **Startup Latency** — eager work on the premain critical path +6. **Domain-Adjusted Severity** — large-denominator domains, streaming handlers + +--- + +## 1. Unbounded Memory (SEV-1 — flag aggressively) + +A collection that grows without a bound can kill the customer's container. The tracer shares the +application heap — there is no isolation. A false positive here is cheap insurance against a +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)`. + +**Cardinality-sensitive aggregator.** A collection with a nominal size bound, but keyed by data +that explodes in cardinality (tag combinations, user-supplied dimensions). High-cardinality input +thrashes the eviction policy — the nominal cap doesn't help. Flag when config or user-driven +values feed such an aggregator without a key-space constraint. + +**Open-cardinality keys.** Any field that varies per-message — timestamp, offset, correlation ID +— used as a key component makes the aggregator grow without bound. Fix: remove the +open-cardinality dimension, or replace raw timestamps with time-buckets. + +**Externally-driven caps.** Any collection grown by Remote Config, user input, or an external +control plane has its growth controlled by the external source. When a PR removes an existing cap +with no replacement bound, flag and ask — the decision may be intentional but must be explicit. + +> **False-positive trap.** Flagging the *absence* of a cache on open-cardinality input (raw SQL +> with inline literals, per-request strings) is wrong. Not caching high-cardinality data *is* the +> correct choice — caching it would be the worse SEV-1. Flag a cache *keyed by* high-cardinality +> data; never flag the decision not to cache. + +*Examples (patterns from back-test calibration):* +- A `LoadingCache` keyed by URL and SQL text with no size or byte limit — the capstone + pattern: unbounded growth tied directly to traffic volume. +- A DSM pathway hash that included a timestamp field, making the aggregator's slot count + grow without bound. Removing the timestamp from the key is a SEV-1 prevention. + +--- + +## 2. Repeated Allocation on Hot Paths (SEV-2/3) + +Tracing is repetitive. Work repeated per-span or per-trace compounds quickly. The focus is on +*allocating* repeat work — patterns that produce garbage the GC must collect — not pure CPU-micro +work like an `.equals()` call. + +**Regex compile per call.** `Pattern.compile(...)` at a non-static site allocates and compiles on +every call. Fix: `static final Pattern`. + +**`String.format` / format-string parsing.** Re-parses and allocates per call. Fix: direct +concatenation, or pre-compute the result. Also watch for locale-dependent formatting crossing the +wire — a correctness issue on top of the perf one. + +**`Config.get()` per call.** Walks a config-resolution chain; not a free read. Fix: hoist to a +`static final` field at class initialization. This is the single most recurring DBM finding — +surfaced independently in five separate PRs. + +**Java Streams on hot paths.** `stream()` and `parallelStream()` always allocate `Spliterator` +and pipeline objects. JIT elision is fragile — small changes to the pipeline or surrounding code +break it silently. Fix: plain `for` loop. Any of `cstyleFor`, `enhancedFor`, `forEach`, or +`iterator` are equivalent and zero-allocation. Benchmark evidence (Java 17, M1, 8 threads, +@Fork(2)): plain loops allocate ≈ 10⁻⁷ B/op; `stream()` always allocates 56–88 B/op; +`parallelStream()` scales from 128 B/op (empty list) to 5 200 B/op (100-element list). + +**`@Advice.AllArguments()`.** Materializes a new `Object[]` boxing all method arguments on every +advised call — always escapes, EA cannot elide it. Fix: `@Advice.Argument(value=N)` for the +specific argument and type needed. + +*Examples (patterns from back-test calibration):* +- A per-trace path with regex compile per call + `String.format` + a locale-dependent formatting + bug — the clearest recall case for mechanism-certain patterns. +- A `StringBuilder(1024)` per query for a ~200-character result. Two-sided error: + under-size causes realloc, over-size wastes memory. Target accurate, not generous. + +--- + +## 3. Per-span Escaping Allocation (SEV-2/3, can reach SEV-1 via tail latency) + +The tracer shares the application heap. Additional allocation contributes to GC and raises +stop-the-world pauses — directly increasing tail latency for the customer's application. The JVM's +escape analysis eliminates *local* short-lived allocations, but only when the object stays local. +Stored in a map, returned, captured by a lambda, or passed to a non-inlined virtual call: it +escapes, and it's real. + +**EA claims for scope/wrapper objects spanning I/O — treat as unverified.** A microbenchmark +tight-loop can show zero allocation for a scope or wrapper object because C2 inlines through +everything and scalar-replaces it. In production, scopes almost always wrap I/O — and C2 cannot +inline through native/blocking I/O boundaries. The object's lifetime extends across the call, it +escapes, and it allocates. A benchmark without I/O-wrapping is not a credible check. Treat EA +claims about per-span scope objects as unverified unless the benchmark explicitly includes +realistic I/O usage. + +**Defensive copies at internal boundaries.** `array.clone()`, `new ArrayList<>(other)` — justified +at real trust boundaries (public API, genuinely mutable external input); wasteful +internal-to-internal where we control all callers. Fix: return a read-only view, or establish a +"don't mutate" contract. + +**Capturing lambda on a hot path.** A non-capturing lambda is a cached singleton — zero alloc. A +capturing lambda (closes over a local or `this`) is a new instance per evaluation. Common trap: +`map.computeIfAbsent(k, k -> compute())` allocates the lambda on *every* call including cache hits +where it is never invoked. Fix: `get` first, `computeIfAbsent` only on miss. + +**`Optional` and primitive boxing.** Any `Optional*` construction allocates per call and escapes. +Autoboxing outside the JVM cache range ([-128, 127] for `Integer`/`Long`) likewise. Fix: null +checks, primitive return values, or fixed-arity overloads. + +*Examples (patterns from back-test calibration):* +- Capturing lambdas allocated per-span to register per-request callbacks. Allocation + accepted: it buys correctness (fixes a span leak). Cost nominates; the do-no-harm hierarchy + adjudicates. +- A per-instance `TagValue` on a per-trace path — the same shape as the regex and format-string + cases above. + +--- + +## 4. Wrong Collection Type (SEV-2/3) + +Three-step ladder: `LinkedHashMap → HashMap → POJO/record`. Lighter wins. + +**`LinkedHashMap` when order isn't relied on.** ~16 B extra per entry + doubly-linked list +maintenance on every put/remove. Only justified when iteration order is required (insertion-order) +or for LRU (`accessOrder` + `removeEldestEntry`). Fix: `HashMap`. + +**`HashMap` for a fixed, small, known key set.** Pays hashing, boxing, and `Entry` object overhead +per lookup. Fix: a plain record or value class — denser, EA-scalar-replaceable when non-escaping, +type-safe. A 5-line record is often *easier* to write than a map. + +**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. + +**Concurrency choice — nominate, don't prescribe.** Replacing `ConcurrentHashMap` with `HashMap` +on a wrong concurrency judgment introduces a data race — trading a performance overhead for a +correctness bug, descending the do-no-harm hierarchy. Frame as a question ("if this map is +thread-confined, a plain collection is cheaper — verify the access pattern"), never a directive. + +*Examples (patterns from back-test calibration):* +- A per-checkpoint `LinkedHashMap` collapsed to a record-like value type. The full + three-step collapse: eliminated per-entry `Entry` overhead, boxing, and linked-list maintenance. + ~20% throughput improvement. +- An oversized `StringBuilder(1024)` is the collection-sizing anti-pattern in a + different form. Accurate sizing, not generous sizing, is the target. + +--- + +## 5. Startup Latency (SEV-2) + +"Once per process" treats startup costs as negligible — but that breaks for serverless (cold starts +are routine), short-lived CI jobs, and deployments that track startup time. + +Flag in premain-reachable code: eager class loading, native library loads (`Native.load`), +reflection setup, config-regex compilation, eager file/network I/O, and thread creation. Fix: +defer to first-use off the hot path, or a background thread post-startup. + +Startup latency and bootstrap correctness share a lens. The bootstrap constraints (no +`java.util.logging` / `java.nio` / `javax.management` in premain) are the correctness side; +startup latency is the performance side. Both route to the platform team — not as contributor +nudges. + +*Examples:* +- `Native.load` inside a `write()` method. If reached on the startup path, it's a + present startup-latency cost (loading libc + building the JNA proxy), not just a latent one. +- **Instrumentation static initializers** — any static field initialization in an `Instrumenter` + subclass that triggers class loading or I/O on first reference is premain-reachable. + +--- + +## 6. Domain-Adjusted Severity + +**Large-denominator domains.** LLMObs, CI Visibility, DSM instrument large units of work (LLM +calls 500 ms+, Spark jobs seconds–minutes, CI test steps milliseconds–minutes). Per-"span" +CPU/alloc severity collapses. But the risk *inverts*: payload memory (large prompts, job metadata, +accumulated output) becomes SEV-1. A CPU-weighted reviewer flags the wrong things and misses the +real one. + +**Streaming handlers — large-denominator rule suspends at the chunk level.** The per-call +denominator applies to costs that fire once per call. Costs inside a streaming handler fire +per-chunk — SSE, chunked HTTP, gRPC streaming can produce hundreds of events per response. An +unbounded accumulator inside a streaming handler (buffering all chunks until stream close) is +SEV-1 regardless of how slow the overall stream is. + +**AppSec sub-domain split.** The WAF blocking path fires only when a block action is triggered — +genuinely cold, SILENT. The IAST taint/sink Reporter can fire frequently during an active security +scan. Treat stream usage and per-call allocations on the IAST Reporter path as SOFT-ALERT, not +cold. Do not apply "AppSec = cold" uniformly across AppSec sub-products. + +*Examples:* +- An LLMObs 5 MB mapper buffer. Same "big buffer" shape as the oversized `StringBuilder` + above, *opposite verdict*: the large-denominator (500 ms+ LLM call) absorbs the cost. + The right call was to accept it. +- An LLMObs streaming helper that accumulated all SSE chunks into an `ArrayList` held + until stream close. The large-denominator rule would have suppressed this; the chunk-level + carve-out catches it: SEV-1, regardless of stream duration. + +--- + +*Companion references in this skill: `checks.md` (the full check list + confidence/severity cost-model + Java addendum) · `example-review.md` (a worked review to calibrate output).* diff --git a/AGENTS.md b/AGENTS.md index e6c0ce4ec90..487fa963bd2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -71,6 +71,14 @@ docs/ Developer documentation (see below) - Use `tag: no release note` for internal/refactoring changes - Open as draft first, convert to ready when reviewable +## 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. + +### 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`. + ## Bootstrap constraints (critical) Code running in the agent's `premain` phase must **not** use: