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."
• Check for shipped evidence before raising any flag-as-measure finding — not just the contestable-tradeoff nudge. Before asking the reader to verify a JIT/GC-dependent cost (J1 escape elision, J4/J7 GC pressure, J5/J11 aggregator cardinality, etc.), check whether the diff itself already ships the answer: a JMH benchmark or profiler run whose stated purpose is exactly this mechanism (e.g. a benchmark class's Javadoc saying "run with -Pjmh.profilers=gc to confirm X is scalar-replaced"), or evidence already recorded in the PR/commit history that the same question was measured. If that evidence directly covers the mechanism, don't re-flag it as open — move it to Correctly suppressed / Checked, no issue and cite the benchmark/evidence by name. Only raise a finding over it if the evidence doesn't actually cover the claimed mechanism (wrong JVM, wrong code path, never actually run) or is stale relative to the current diff.
• 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:

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.