perf-review

# 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. ## 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 <X for Y>; 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–3 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 — ). Showing your suppressed lookalikes and what you cleared is not filler: it demonstrates the precision that makes the findings trustworthy. When providing suggestions as code review comments, prefix the comments with "perf: " ```markdown # Perf Review — <branch / PR> **Scope reviewed:** <the hot method(s) and why they're hot — the multiplier> ## Confirmed findings ### 1. <one-line title> <the offending code, as a short fenced snippet> - **Confidence:** flag-with-confidence | flag-as-measure - **Reachability:** <hot from X via A→B→C, no guard on that path> - **Rubric check:** <#N / JN> - **Severity:** SEV-<n> - **Fix / verify-with:** <the actionable fix, or "verify with an allocation profiler"> ## Correctly suppressed (not flagged) <textual lookalikes deliberately left silent — e.g. the same `Objects.hash` pattern but at class-init (cold), not per-call — and why the posture suppresses them> ## Checked, no issue <what you examined and cleared: e.g. "no unbounded cache (#3/J5)", "no native crossing (#6/J3)", "string-literal tag keys are JVM-interned — no per-call alloc"> ## Summary <count + severity spread; e.g. "4 confirmed hot-path findings, all SEV-2/3 (allocation/CPU); 1 cold-path lookalike correctly suppressed"> ``` 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.