Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion .agents/skills/perf-review/references/checks.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,17 @@ Fixed-signature, mechanically checkable:
- 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)
- a field (instance or static, directly or as a generic type argument) declared with an
`@NoEscape`-annotated type (`datadog.trace.api.function.NoEscape`), with no comment at the
declaration justifying the retention — the annotation's own javadoc carries a self-contained
"Checker contract" section (trigger / not-a-trigger / violation example / compliant example)
written so this can be checked from the diff alone, with no other context needed. The
underlying rule is "should", not "must" (RFC-2119 sense): a trigger is a presumptive
finding, not an automatic failure — a field with a `// Retained on purpose: <reason>`-style
comment is compliant. **flag-with-confidence** — SEV-2/3 (SEV-1 if the annotated type shares
backing storage with something large, per its own javadoc). Current wearers: `SubSequence`,
`Maybe` (see J7 below for `SubSequence`'s specific retention-vs-transient
discriminator).
- *(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**)
Expand All @@ -45,7 +56,7 @@ Refines the universal checks with JVM mechanics. Quarantined here, for the Java
- **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).
- **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). `SubSequence` carries `@NoEscape` — see the deterministic-lint entry above for the general field-storage check this refines.
- **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)*. The allocation is specific to the **varargs/boxing forms**: `Objects.hash(a, b, …)` allocates an `Object[]` per call and **boxes every primitive** arg; same for boxing primitives into a `new Object[]{…}` (or `Arrays.hashCode` over such an array). Per-span tag/key building, or a hot value object's `hashCode()` built this way, → 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 for the varargs/boxing form — SEV-2/3. **Do NOT flag allocation-free combines** — a hand-rolled `31*h + Long.hashCode(x)` / `31*h + intField`, or `Arrays.hashCode` over an *existing primitive array*, allocates nothing (`HashingUtils` is itself 31-based); flagging them would recommend replacing already-correct code.
- **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.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package datadog.trace.api.function;

import java.lang.annotation.Documented;
import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

/**
* Marks a type that should never be <em>retained</em> -- never assigned to a field, put into a
* collection, or cached -- though it may otherwise flow normally through ordinary code: returned
* from a method, passed to a callback, chained through further calls. The line is storage, not how
* far the value travels: a {@code @NoEscape} value can cross many methods and frames as long as
* nothing along the way parks it somewhere that outlives the operation using it.
*
* <p>"Should", in the RFC-2119 sense: retaining an instance is presumed wrong and needs a reason,
* not an absolute prohibition. A deliberate, reviewed exception -- e.g. a container that retains a
* {@code @NoEscape} value as a precaution and has weighed the tradeoff -- is legitimate as long as
* it is called out at the retention site (e.g. a comment explaining why) rather than done silently.
*
* <p>Two motivating shapes, both real for existing types in this codebase:
*
* <ul>
* <li><b>Shared-backing view.</b> A type that shares backing storage with something it was
* derived from (e.g. a substring view that shares its parent {@code String}'s backing array)
* to avoid a copy. Storing an instance pins the entire backing object alive for as long as
* the view is retained, turning an allocation-avoidance trick into a memory leak the moment
* it is kept around rather than consumed and dropped.
* <li><b>Escape-analysis-dependent value.</b> A type deliberately shaped to scalar-replace under
* escape analysis rather than actually allocate, on the assumption that it is constructed,
* consumed, and discarded rather than stored. Assigning an instance to a field or collection
* forces the JIT to materialize it as a real, permanent allocation, defeating the reason the
* type exists -- even though passing the same instance through several method calls first is
* fine.
* </ul>
*
* <p>This is a documentation-and-tooling marker; it changes no behavior. It exists to telegraph the
* constraint to readers and to give a future checker something to verify -- that a field (instance
* or static) declared with a {@code @NoEscape} type has a reason to be there. The discipline it
* names is <b>not yet enforced</b>; hold to it by hand until the checker lands.
*
* <p><b>On a type</b> ({@link ElementType#TYPE}): instances of this type should not be stored in a
* field or collection. Returning one, passing it to a callback, or chaining further calls on it is
* fine -- what needs a reason is anything that keeps it alive past the operation using it.
*
* <p><b>Checker contract.</b> The rule below is written to be machine-checkable -- by a future
* static checker, or in the meantime by an AI reviewer (see the perf-review skill's {@code
* checks.md}) -- without needing to read this class's prose above. Because the underlying rule is
* "should" rather than "must", a trigger is a presumptive finding to raise, not an automatic
* failure: a field that carries a comment explaining the deliberate exception is compliant.
*
* <ul>
* <li><b>Trigger:</b> a field (instance or static, in any class) whose declared type is annotated
* {@code @NoEscape}, either directly (e.g. {@code SubSequence field;}) or as a generic type
* argument of the field's declared type (e.g. {@code List<SubSequence>}, {@code Map<K,
* Maybe<V>>}), with no comment at the declaration explaining why the retention is safe.
* <li><b>Not a trigger:</b> a local variable, a method parameter, or a method return type -- this
* rule flags <em>storage</em> that outlives the call, not ordinary use within it. Also not a
* trigger: the same field shape, annotated with a comment justifying the retention.
* <li><b>Violation example:</b> {@code private final SubSequence cached;}
* <li><b>Compliant example:</b> {@code private final String cached;} -- materialize the view
* (e.g. call {@code toString()}) before storing it. Or, if retention is a deliberate,
* reviewed exception: {@code // Retained on purpose: <reason>} above the field.
* <li><b>Out of scope (v1):</b> escape through a non-generic/raw container, a capturing lambda,
* or a returned value the caller goes on to store. Flag only the field-declaration shape
* above; widen this contract only once a real case proves it insufficient, rather than
* guessing ahead of one.
* </ul>
*/
@Documented
@Retention(RetentionPolicy.SOURCE)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve marker metadata for dependent modules

When the promised checker analyzes a Gradle module that consumes :internal-api, it sees SubSequence and Maybe from the compiled artifact rather than their source files. SOURCE retention removes @NoEscape from those class files, so the checker cannot determine that a dependent module's field uses an annotated type and such violations silently evade enforcement. Use CLASS retention, which remains unavailable to runtime reflection, or require the checker to index all source trees globally.

Useful? React with 👍 / 👎.

@Target(ElementType.TYPE)
public @interface NoEscape {}
8 changes: 8 additions & 0 deletions internal-api/src/main/java/datadog/trace/util/Maybe.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package datadog.trace.util;

import datadog.trace.api.function.NoEscape;
import java.util.function.BiConsumer;
import java.util.function.Consumer;
import java.util.function.Function;
Expand Down Expand Up @@ -33,7 +34,14 @@
* phiOfTwoAllocations} arm, which uses two distinct interface implementations rather than one
* concrete type and therefore fails to scalar-replace on every JDK including 25 -- a different,
* stronger failure mode than the one demonstrated here.
*
* <p>{@link NoEscape}: the scalar-replacement discipline above only holds while a {@code Maybe} is
* constructed, consumed (typically via {@link #update}/{@link #getOrNull}), and discarded rather
* than stored -- returning one, or passing it along to be consumed further downstream, is fine.
* Assigning an instance to a field or collection forces the JIT to materialize it as a real,
* permanent allocation, defeating the reason it exists.
*/
@NoEscape
public final class Maybe<T> {
@Nullable private final T value;

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package datadog.trace.util;

import datadog.trace.api.function.NoEscape;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

/**
Expand All @@ -14,7 +15,12 @@
* (an offset + length into the existing backing array), so the same parse allocates nothing per
* slice. Use it for transient, read-only views; materialize a real <code>String</code> only when
* the value must be retained or handed off.
*
* <p>{@link NoEscape}: because a <code>SubSequence</code> shares its parent <code>String</code>'s
* backing array, holding one anywhere longer-lived than the call that produced it (a field, a
* cache, a collection) pins the entire parent string alive for as long as the view survives.
*/
@NoEscape

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve the annotated EMPTY singleton

Applying @NoEscape here immediately contradicts its checker contract: the next line declares the static field SubSequence EMPTY, while the contract explicitly triggers on any instance or static field whose declared type is annotated. A checker implementing the documented rule will therefore report this class itself even though the singleton only retains an empty string; remove the field or define and implement a narrow safe-singleton exception so the rule does not begin with a known false positive.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Justify the retained EMPTY singleton

The rule creates a false finding for its first annotated type and reduces review accuracy.

Assertion details
  • Input: Run the new @NoEscape field-storage check against SubSequence.EMPTY.
  • Expected: Add a justification comment to EMPTY, or define an explicit exemption for safe singleton fields.
  • Actual: The new review rule flags the safe SubSequence.EMPTY static field because it stores an annotated type without a justification comment.

Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session

public final class SubSequence implements CharSequence {
public static final SubSequence EMPTY = new SubSequence("", 0, 0);

Expand Down