diff --git a/.agents/skills/perf-review/references/checks.md b/.agents/skills/perf-review/references/checks.md index cdf601f4083..a92979d7724 100644 --- a/.agents/skills/perf-review/references/checks.md +++ b/.agents/skills/perf-review/references/checks.md @@ -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: `-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**) @@ -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. diff --git a/internal-api/src/main/java/datadog/trace/api/function/NoEscape.java b/internal-api/src/main/java/datadog/trace/api/function/NoEscape.java new file mode 100644 index 00000000000..8408709367a --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/function/NoEscape.java @@ -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 retained -- 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. + * + *

"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. + * + *

Two motivating shapes, both real for existing types in this codebase: + * + *

+ * + *

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 not yet enforced; hold to it by hand until the checker lands. + * + *

On a type ({@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. + * + *

Checker contract. 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. + * + *

+ */ +@Documented +@Retention(RetentionPolicy.SOURCE) +@Target(ElementType.TYPE) +public @interface NoEscape {} diff --git a/internal-api/src/main/java/datadog/trace/util/Maybe.java b/internal-api/src/main/java/datadog/trace/util/Maybe.java index 5c0a1fb0499..4aec3fe290e 100644 --- a/internal-api/src/main/java/datadog/trace/util/Maybe.java +++ b/internal-api/src/main/java/datadog/trace/util/Maybe.java @@ -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; @@ -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. + * + *

{@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 { @Nullable private final T value; diff --git a/internal-api/src/main/java/datadog/trace/util/SubSequence.java b/internal-api/src/main/java/datadog/trace/util/SubSequence.java index abac3ea6a7c..5cc3b257168 100644 --- a/internal-api/src/main/java/datadog/trace/util/SubSequence.java +++ b/internal-api/src/main/java/datadog/trace/util/SubSequence.java @@ -1,5 +1,6 @@ package datadog.trace.util; +import datadog.trace.api.function.NoEscape; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; /** @@ -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 String only when * the value must be retained or handed off. + * + *

{@link NoEscape}: because a SubSequence shares its parent String'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 public final class SubSequence implements CharSequence { public static final SubSequence EMPTY = new SubSequence("", 0, 0);