diff --git a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md index 9ad44dc33f9..546602a91dc 100644 --- a/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md +++ b/.agents/dd-apm-sdk-review-overrides/reviewers/performance.md @@ -1,6 +1,6 @@ Override for `reviewers/performance.md` (in the core skill folder) — read that file first, then this. -Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md,checks.md}` (the previously-shipped, now-retired Java performance rubric; the skill folder was removed to avoid two competing performance-review entry points). Everything language-agnostic from that rubric moved to the core `reviewers/performance.md` (confidence axis, severity model, domain-adjusted severity, universal checks); everything Java-specific is the addenda below (J1-J15), the instrumentation idioms, the deterministic-lint candidates, and the bootstrap note. +Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md,checks.md}` (the previously-shipped, now-retired Java performance rubric; the skill folder was removed to avoid two competing performance-review entry points). Everything language-agnostic from that rubric moved to the core `reviewers/performance.md` (confidence axis, severity model, domain-adjusted severity, universal checks); everything Java-specific is the addenda below (J1-J16), the instrumentation idioms, the deterministic-lint candidates, and the bootstrap note. # Performance — dd-trace-java specifics @@ -30,10 +30,22 @@ Seeded from this repo's former `.agents/skills/perf-review/references/{guide.md, - **J13 — Defensive copies at internal boundaries** *(refines universal `per-call-allocation`)*. `array.clone()`, `new ArrayList<>(other)`, and similar copies are justified at a real trust boundary (a public API, or genuinely mutable external input), but also when the callee needs a stable snapshot across threads or ownership of the data — a read-only view only blocks mutation *through that view* and still reflects later changes to the backing collection, so swapping in a view where a real copy was needed trades an allocation for an aliasing/concurrency bug. flag-with-confidence only when neither a trust boundary nor a stable-snapshot/ownership-transfer need is established — SEV-2/3. Fix: return a read-only view, or establish a "don't mutate" contract instead of copying. - **J14 — Capturing lambda allocated on every call, including cache hits** *(refines universal `per-call-allocation`, `escape-elision-defeated`)*. 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 — but when the call site is inlined and the mapping function isn't retained past the `computeIfAbsent` call, escape analysis can still scalar-replace it on a cache hit. The recurring trap: `map.computeIfAbsent(k, k -> compute())` looks like it allocates the lambda on *every* call including cache hits, but whether it actually survives is optimizer-dependent. flag-as-measure — verify in JFR/an allocation profile that the lambda escapes before treating it as a confirmed cost; only escalate to flag-with-confidence once escape is established (e.g. the mapping function is stored, passed to a virtual call, or the call site is megamorphic) — SEV-2/3. Fix: `get` first, call `computeIfAbsent` only on a miss. - **J15 — `Optional` construction and primitive boxing outside the JVM cache range** *(refines universal `per-call-allocation`)*. `Optional.empty()` / `OptionalInt.empty()` / `OptionalLong.empty()` / `OptionalDouble.empty()` return cached singletons and don't allocate. A non-empty `Optional*` construction allocates a new instance, but C2 can scalar-replace it when the value is created and immediately consumed in an inlined method — same as J14's local-object case. flag-with-confidence only when the Optional is returned, stored in a field/collection, or otherwise proven to escape; flag-as-measure for local create-and-consume (verify in JFR before treating it as a confirmed cost). Autoboxing a primitive outside the JVM's cached range (`[-128, 127]` for `Integer`/`Long`) likewise allocates when the boxed value escapes: flag-with-confidence then, flag-as-measure for a locally consumed box. Fix: null checks or primitive-typed return values instead of `Optional`; fixed-arity primitive overloads instead of boxing. SEV-2/3. +- **J16 (slug: `background-only-contract`) — `@ForegroundSafe`/`@BackgroundOnly` declared-contract reachability** *(distinct axis — not a refinement of a universal cost check; a declared-contract violation, not an inferred cost)*. `datadog.trace.api.function.ForegroundSafe` and `.BackgroundOnly` (`CLASS`-retention markers) declare a one-way compatibility contract: `@ForegroundSafe` code must be cheap enough to call from either an application (foreground) thread or a background thread; `@BackgroundOnly` code must never be reached from a foreground call site. A method-level annotation always overrides a type-level one on the same element. Flag-with-confidence a diff that adds a call path from `@ForegroundSafe`-or-unannotated foreground code (a span lifecycle method, an instrumentation advice body, anything on the app thread) into a symbol declared `@BackgroundOnly`. + + **Inheritance direction is a second, independent trigger — checked at the declaration, not the call site.** The full rule and rationale live in `BackgroundOnly`'s Javadoc ("Inheritance direction" section, mirrored in `ForegroundSafe`'s) — don't restate it here, just apply it: an override may only narrow a supertype's declared cost, never widen it. The checker-facing consequence: overriding a `@ForegroundSafe` or unannotated supertype method and marking the override `@BackgroundOnly` is flag-with-confidence the moment it appears in the diff, whether or not any foreground call site currently reaches it — the reverse direction (narrowing `@BackgroundOnly` to `@ForegroundSafe`) is compliant, not a finding. + + Severity follows this lens's normal cost-based ladder (§ "Mapping SEV to this skill's P0/P1/P2 scale" above), not a fixed level — the contract crossing tells you *that* the callee's cost is now paid on the wrong thread, not by itself how large that cost is: SEV-2/3 by default (general tracer/background-style overhead moved to the app thread); escalate toward SEV-1/2 when the reached call site is itself on a hot per-span/per-request path, or when the background-only code also does I/O, blocking, or heavy allocation, matching the general "hot-path multiplier" and denominator guidance above. Resolution is grep-only today (`grep -rn "@BackgroundOnly\|@ForegroundSafe"`), including across module boundaries when the callee lives outside the diff — there is no APT-generated manifest yet, so cross-module resolution costs a grep per unfamiliar symbol rather than a lookup (tracked in APMLP-1645). **This is local type-tracking (creation point to usage point within a class/file), not cross-file data-flow.** Extend resolution exactly two hops beyond the direct case, both anchored to files already in hand — never the reverse (enumerating every implementer of an interface to see whether any one of them is `@BackgroundOnly`); that search is unbounded, repo-wide, and belongs to APMLP-1645's real resolver, not this grep-based check: + + 1. **Concrete → declared interface, method-scoped.** When a foreground call site is typed to an interface `I` rather than the concrete class, and some implementation `C` of `I` — already in hand, i.e. visible in the diff or files handed to you, never found by searching the repo for implementers — is `@BackgroundOnly` (type- or method-level), read `C`'s `implements`/`extends` clause and match `C`'s `@Override` methods against `I`'s method signatures. Propagate the marker only to *that signature* on `I` — not to every method `I` declares. A foreground call to a different, unrelated method on `I` is not a finding. **First check whether the call site's receiver is visibly bound in the diff** (a local `new`/factory-method assignment, a constructor/field initializer, a DI wire-up you can see) **to a specific implementation of `I` other than `C`.** When it is, and that other implementation is not itself `@BackgroundOnly`, resolve against what's actually visible instead of `C` — this is not a finding, since `C` cannot be what that particular reference calls. Reserve hop 1 for the case where the receiver's concrete type isn't visible in hand (a parameter, a field populated elsewhere, a return value from outside the diff) and `C` is the only candidate implementation you have to reason from. + 2. **Default method transitively calling a flagged signature.** If `I` declares a default method `d()` whose body calls one of the signatures flagged in step 1, `d()` inherits the flag too. This is a single read of `I`'s own source — do not chase the call further than one interface file. + + Neither hop proves the call site's interface reference actually resolves to `C` at runtime when the receiver's concrete type isn't visible (that's still unprovable by grep) — treat a hit from either hop as flag-with-confidence in that case, same as the direct case, since a call site that would only ever reach a *safe* implementer of `I` is the rare case, not the default one to assume absent evidence. But when the diff *does* show the receiver bound to a specific, different, safe implementation, that visible evidence overrides the default assumption — don't flag against a `C` the call site provably can't reach. + +**Stay silent, not "probably fine," on an unclassified symbol** — the absence of either annotation on the callee is not itself a finding; only a diff that *provably* crosses a declared `@BackgroundOnly` boundary from a foreground path counts. For a feel of the shape (not an exhaustive list — grep for the current, complete set): `FixedSizeCache` carries `@ForegroundSafe` (bounded-probe implementation only — the `DDCache` interface itself and its other implementations, e.g. `CHMCache`, are deliberately unannotated since their cost isn't uniformly bounded), `SimpleUtf8Cache` carries `@BackgroundOnly`. Fix: remove the call from the foreground path, or move the enclosing work to the background/serializer thread that legitimately owns the `@BackgroundOnly` code. **J7–J11 route an *existing* universal `per-call-allocation`/`repeat-work-across-calls`/`unbounded-memory` finding to a landed reusable fix — they are not new triggers.** Don't raise a finding you wouldn't have raised anyway. -**Toolkit availability — cite only what exists.** Available today: `Strings`, `SubSequence`, `HashingUtils`, `Hashtable`, `ConcurrentHashtable`, `StringIndex` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`). Coming (name as "coming", don't imply present): `UTF8BytesString.Cache`, wider `IntegerCache`, `DDCache` inlining. +**Toolkit availability — cite only what exists.** Available today: `Strings`, `SubSequence`, `HashingUtils`, `Hashtable`, `ConcurrentHashtable`, `StringIndex` (all `datadog.trace.util`), `RE2J` (`com.google.re2j`), `ForegroundSafe`/`BackgroundOnly` (`datadog.trace.api.function`, scoped only to whatever J16's grep currently turns up — see J16 above, don't assume repo-wide coverage). Coming (name as "coming", don't imply present): `UTF8BytesString.Cache`, wider `IntegerCache`, `DDCache` inlining. ## Instrumentation (ByteBuddy Advice) idioms — dd-trace-java-specific fixes diff --git a/.llm-validation/README.md b/.llm-validation/README.md index 00dcf8202fd..be299ca5176 100644 --- a/.llm-validation/README.md +++ b/.llm-validation/README.md @@ -56,7 +56,7 @@ docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ --repo /repo --base-sha master --level full --runs 1 -# CI-shaped set (8 cases) +# CI-shaped set (13 cases) docker run --rm -e LLMVAL_AUTH_HEADER -v "$PWD:/repo" "$LLMVAL_IMAGE" \ --repo /repo --base-sha master --level gate --runs 1 @@ -110,7 +110,7 @@ that level already selected. | Level | Cases | Default runs | Use | |---|---|---|---| | `minimum` | **1** (`java-perf-lens-wrong-collection-001`) | 2 | First smoke | -| `gate` (default) | **8** listed in `config.yaml` | 2 | CI-shaped | +| `gate` (default) | **13** listed in `config.yaml` | 2 | CI-shaped | | `full` | **every** case in `suites/` | 2 | Broader pass | So this command runs **one** case once, not the whole suite: diff --git a/.llm-validation/config.yaml b/.llm-validation/config.yaml index 1e4632ba076..81e4b195a4b 100644 --- a/.llm-validation/config.yaml +++ b/.llm-validation/config.yaml @@ -33,6 +33,11 @@ presets: - java-maintainability-resource-leak-streams - java-correctness-span-events-list-only - java-correctness-mapper-state-leak + - java-perf-foreground-calls-background-only-001 + - java-perf-foreground-calls-unannotated-002 + - java-perf-foreground-calls-method-override-003 + - java-perf-foreground-calls-interface-hop-004 + - java-perf-inheritance-widen-violation-005 runs: 2 minimum: cases: ["java-perf-lens-wrong-collection-001"] diff --git a/.llm-validation/suites/dd-apm-sdk-review.yaml b/.llm-validation/suites/dd-apm-sdk-review.yaml index 2cd716ed13e..eb9cdc07c2a 100644 --- a/.llm-validation/suites/dd-apm-sdk-review.yaml +++ b/.llm-validation/suites/dd-apm-sdk-review.yaml @@ -368,3 +368,234 @@ cases: - "Notes the flag exists but misses that nothing re-initializes it per payload." - "Suggests resetting the field at the start of every `map()` (or only \"make it volatile\" / \"add logging\")." - "Treats this as a crash rather than silent data loss from leaked state." + + - id: java-perf-foreground-calls-background-only-001 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. + + ```java + // datadog/communication/serialization/SimpleUtf8Cache.java (hypothetical shape, for + // illustration — annotation and bookkeeping rationale match the real class; the no-arg + // constructor and SHARED field below are simplified for this example) + /** + * {@link BackgroundOnly}: bookkeeping to reuse the encoded byte[] only pays off when this + * is confined to the single background serializer thread that owns it. + */ + @BackgroundOnly + public final class SimpleUtf8Cache implements EncodingCache { + public static final SimpleUtf8Cache SHARED = new SimpleUtf8Cache(); + + public byte[] getUtf8(String value) { /* ... */ } + } + ``` + + ```diff + diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java + @@ -95,6 +95,11 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + public static class HttpUrlConnectionAdvice { + + private static final String INTEGRATION_NAME = "http-url-connection"; + + + + // PR #99999: pre-encode the integration name once so debug logging doesn't re-encode it. + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + + public static void onExit(@Advice.This final HttpURLConnection thiz) { + + byte[] utf8 = SimpleUtf8Cache.SHARED.getUtf8(INTEGRATION_NAME); + + log.debug("traced {} bytes via connection", utf8.length); + + } + ``` + expected_criteria: + - "Flags that `onExit` — Byte Buddy advice running on the application thread for every instrumented `HttpURLConnection` call — reaches `SimpleUtf8Cache.getUtf8(...)`, a class declared `@BackgroundOnly`, per J16's declared-contract check." + - "Escalates severity toward SEV-1/2 (mapping to P1, absent OOM/SLA evidence) because the call site is a hot per-request path (every instrumented HttpURLConnection call), per J16's graduated severity guidance and the core SEV→P mapping — not a hardcoded top severity independent of call-site frequency." + - "Recommends removing the call from advice or moving the work to the background/serializer thread that legitimately owns `SimpleUtf8Cache`, rather than caching the value differently in place." + bad_signals: + - "Treats this as an ordinary allocation or contention finding without naming the declared `@BackgroundOnly` contract violation." + - "Assumes the cache is safe to call from advice because it is described as a \"cache\" without checking the annotation." + - "Misses that `onExit` runs on the application/foreground thread, not the serializer thread." + - "Dismisses or downgrades the contract violation itself (e.g. calls it optional, stylistic, or something to fix \"eventually\") instead of treating a proven `@BackgroundOnly` crossing as a real, actionable finding regardless of which P-level it lands on." + + - id: java-perf-foreground-calls-unannotated-002 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. + + ```java + // datadog/communication/serialization/LookupCache.java (hypothetical, for illustration) + public final class LookupCache implements EncodingCache { + public static final LookupCache SHARED = new LookupCache(); + + public byte[] getUtf8(String value) { /* ... */ } + } + ``` + + ```diff + diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java + @@ -95,6 +95,11 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + public static class HttpUrlConnectionAdvice { + + private static final String INTEGRATION_NAME = "http-url-connection"; + + + + // PR #99998: pre-encode the integration name once so debug logging doesn't re-encode it. + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + + public static void onExit(@Advice.This final HttpURLConnection thiz) { + + byte[] utf8 = LookupCache.SHARED.getUtf8(INTEGRATION_NAME); + + log.debug("traced {} bytes via connection", utf8.length); + + } + ``` + expected_criteria: + - "Does not raise a J16 declared-contract finding for this call — `LookupCache` carries neither marker annotation, and J16 is explicit that the absence of an annotation on the callee is not itself a finding." + - "If the response mentions `LookupCache` at all, it either says plainly that it is unclassified/unannotated and out of J16's scope, or raises a *different*, independently-justified finding (e.g. an ordinary per-call-allocation or hot-path concern under another addendum) rather than treating this as a `@BackgroundOnly` contract violation." + bad_signals: + - "Flags this as a `@BackgroundOnly` (or `@ForegroundSafe`) contract violation despite `LookupCache` carrying no such annotation." + - "Assumes `LookupCache` is background-only by analogy to `SimpleUtf8Cache` (same shape, similar name) rather than checking for the actual annotation." + - "Treats the mere presence of a cache-like class called from advice as sufficient grounds for a declared-contract finding, independent of whether any marker annotation is actually present." + + - id: java-perf-foreground-calls-method-override-003 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. + + `HttpUrlConnectionAdvice.onExit` is Byte Buddy advice, which always runs synchronously on + the calling application thread — for every instrumented `HttpURLConnection` request — and + calls only `sizeHint()`, never any other method on `StatsCache`. + + ```java + // datadog/communication/serialization/StatsCache.java (hypothetical, for illustration) + @BackgroundOnly + public final class StatsCache { + public static final StatsCache SHARED = new StatsCache(); + + @ForegroundSafe + public int sizeHint() { /* O(1) volatile field read, no allocation */ } + + public void recordSample(long value) { /* background bookkeeping */ } + } + ``` + + ```diff + diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java + @@ -95,6 +95,10 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + public static class HttpUrlConnectionAdvice { + + // PR #99997: size the debug-log buffer up front using the cache's current size hint. + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + + public static void onExit(@Advice.This final HttpURLConnection thiz) { + + log.debug("cache sizeHint={}", StatsCache.SHARED.sizeHint()); + + } + ``` + expected_criteria: + - "Does not raise a J16 declared-contract finding for this call — `sizeHint()` carries its own method-level `@ForegroundSafe`, which per J16 overrides the enclosing type's `@BackgroundOnly` for that method specifically, so calling it from foreground advice is compliant, not a violation." + - "If it discusses the annotations at all, correctly states that method-level markers win over type-level ones, and that this is exactly the compliant case, not merely an unclassified or borderline one." + bad_signals: + - "Flags this as a `@BackgroundOnly` violation by looking only at the class-level annotation on `StatsCache` and ignoring the method-level `@ForegroundSafe` on `sizeHint()`." + - "Treats the presence of `@BackgroundOnly` anywhere on the type as disqualifying every method on it, contradicting J16's explicit method-overrides-type rule." + - "Recommends removing or moving the `sizeHint()` call as if it were a real contract crossing." + + - id: java-perf-foreground-calls-interface-hop-004 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. + + ```java + // datadog/communication/serialization/TagCache.java (hypothetical, for illustration) + public interface TagCache { + byte[] encode(String s); + } + ``` + + ```java + // datadog/communication/serialization/StatsTagCache.java (hypothetical, for illustration) + @BackgroundOnly + public final class StatsTagCache implements TagCache { + public static final StatsTagCache SHARED = new StatsTagCache(); + + @Override + public byte[] encode(String s) { /* background-only bookkeeping */ } + } + ``` + + ```diff + diff --git a/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java b/dd-java-agent/instrumentation/java/java-net/java-net-1.8/src/main/java/datadog/trace/instrumentation/java/net/HttpUrlConnectionInstrumentation.java + @@ -95,6 +95,11 @@ public class HttpUrlConnectionInstrumentation extends InstrumenterModule.Tracing { + public static class HttpUrlConnectionAdvice { + + private static final String INTEGRATION_NAME = "http-url-connection"; + + // PR #99996: cache is referenced through its interface type here, not the concrete class. + + private static final TagCache CACHE = StatsTagCache.SHARED; + + + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + + public static void onExit() { + + byte[] utf8 = CACHE.encode(INTEGRATION_NAME); + + } + ``` + expected_criteria: + - "Flags this as a J16 declared-contract violation even though the call site (`CACHE.encode(...)`) is typed to the unannotated interface `TagCache`, not the `@BackgroundOnly` concrete class `StatsTagCache` — by reading `StatsTagCache`'s `implements TagCache` clause and matching its `@Override encode(String)` against `TagCache.encode(String)`, per J16's concrete-to-interface, method-scoped propagation hop." + - "Identifies the `onExit` advice body as the foreground call site, since Byte Buddy advice always runs synchronously on the application thread." + bad_signals: + - "Stays silent because the declared type of `CACHE` (`TagCache`) carries no annotation, without checking whether a `@BackgroundOnly` implementer of that interface is present in the same files." + - "Treats this as unresolvable because the call site isn't textually adjacent to the `@BackgroundOnly` annotation, instead of applying the one-hop concrete-to-interface propagation J16 describes." + - "Over-generalizes by claiming every method on `TagCache` is now background-only, rather than propagating the flag only to the specific `encode(String)` signature `StatsTagCache` overrides." + + - id: java-perf-inheritance-widen-violation-005 + files: + - .agents/skills/dd-apm-sdk-review/reviewers/_common.md + - .agents/skills/dd-apm-sdk-review/reviewers/performance.md + - .agents/dd-apm-sdk-review-overrides/reviewers/performance.md + input: | + Apply this repo's performance-review criteria (specifically the J16 declared-contract + addendum in .agents/dd-apm-sdk-review-overrides/reviewers/performance.md, plus + reviewers/_common.md) to the following change. No git checkout available — this diff is + the entire change to review. There is no foreground call site to this new override anywhere + in this diff or elsewhere in the codebase yet. + + ```java + // datadog/communication/serialization/TagCache.java (hypothetical, for illustration) + public interface TagCache { + @ForegroundSafe + byte[] encode(String s); + } + ``` + + ```diff + diff --git a/datadog/communication/serialization/HeavyTagCache.java b/datadog/communication/serialization/HeavyTagCache.java + new file mode 100644 + @@ -0,0 +1,10 @@ + +package datadog.communication.serialization; + + + +public final class HeavyTagCache implements TagCache { + + + + @Override + + @BackgroundOnly + + public byte[] encode(String s) { + + // heavy bookkeeping, safe only from the background serializer thread + + return null; + + } + +} + ``` + expected_criteria: + - "Flags this as a J16 declared-contract violation at the declaration site itself — `HeavyTagCache.encode` overrides `TagCache.encode`, which is `@ForegroundSafe`, and marks the override `@BackgroundOnly`, which widens the contract rather than narrowing it." + - "States plainly that this finding does not require any current foreground call site to exist — every existing and future caller holding a `TagCache`-typed reference already assumed the `@ForegroundSafe` guarantee and would be broken by this override, per J16's inheritance-direction rule." + bad_signals: + - "Stays silent because no foreground call site reaches `HeavyTagCache.encode` yet, treating the declaration-site rule as if it only applied once a call path exists." + - "Approves this as a normal narrowing override (confusing the direction), missing that narrowing only goes the other way: `@BackgroundOnly` supertype to `@ForegroundSafe` override is fine, the reverse is not." + - "Flags a different, unrelated concern (e.g. plain per-call allocation) without ever naming the inheritance-direction violation that J16 specifically calls out." diff --git a/communication/src/main/java/datadog/communication/serialization/EncodingCache.java b/communication/src/main/java/datadog/communication/serialization/EncodingCache.java index 4833eb8695a..d0b6bc086b3 100644 --- a/communication/src/main/java/datadog/communication/serialization/EncodingCache.java +++ b/communication/src/main/java/datadog/communication/serialization/EncodingCache.java @@ -1,6 +1,13 @@ package datadog.communication.serialization; +import datadog.trace.api.function.BackgroundOnly; + // TODO @FunctionalInterface +/** + * {@link BackgroundOnly}: every implementation ({@link SimpleUtf8Cache}, {@link + * GenerationalUtf8Cache}) is only safe to call from the writer/serializer thread that owns it. + */ +@BackgroundOnly public interface EncodingCache { byte[] encode(CharSequence s); diff --git a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java index 099ff56e0e0..d0911907116 100644 --- a/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/GenerationalUtf8Cache.java @@ -1,5 +1,6 @@ package datadog.communication.serialization; +import datadog.trace.api.function.BackgroundOnly; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.nio.charset.StandardCharsets; import javax.annotation.concurrent.ThreadSafe; @@ -15,6 +16,11 @@ * String#getBytes(java.nio.charset.Charset)}. * *

The cache is thread safe. + * + *

{@link BackgroundOnly}: the eden/tenured promotion and recalibration bookkeeping costs more + * than the {@code getBytes} call it replaces, so the saving only shows up as reduced allocation/GC + * pressure on the thread that pays it -- confine it to the background serializer thread, not an + * application thread. */ /* * Cache works by using a 2-level promotion based scheme. @@ -64,6 +70,7 @@ * provide better cache utilization. */ @ThreadSafe +@BackgroundOnly @SuppressFBWarnings( value = "IS2_INCONSISTENT_SYNC", justification = diff --git a/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java b/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java index 8eb12d48465..2398c4d4e05 100644 --- a/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java +++ b/communication/src/main/java/datadog/communication/serialization/SimpleUtf8Cache.java @@ -1,5 +1,6 @@ package datadog.communication.serialization; +import datadog.trace.api.function.BackgroundOnly; import java.nio.charset.StandardCharsets; import javax.annotation.concurrent.ThreadSafe; @@ -13,6 +14,11 @@ * String#getBytes(java.nio.charset.Charset)}. * *

The cache is thread safe. + * + *

{@link BackgroundOnly}: the bookkeeping (hit counting, LFU eviction scan) costs more than the + * {@code getBytes} call it replaces, so the saving only shows up as reduced allocation/GC pressure + * on the thread that pays it -- confine it to the background serializer thread, not an application + * thread. */ /* * Thread safety is achieved through using CacheEntry objects where the key data @@ -43,6 +49,7 @@ * a LFU: least frequently used eviction policy is used to free up a slot. */ @ThreadSafe +@BackgroundOnly public final class SimpleUtf8Cache implements EncodingCache { static final int MAX_CAPACITY = 1024; diff --git a/docs/instrumentation_design_guidelines.md b/docs/instrumentation_design_guidelines.md index 305ff651439..6e4676e49d1 100644 --- a/docs/instrumentation_design_guidelines.md +++ b/docs/instrumentation_design_guidelines.md @@ -81,3 +81,19 @@ Context ctx = Context.current(); // GOOD - use the bytecode bridge, static-imported Context ctx = currentContext(); ``` + +### 4. Foreground/background thread cost contract + +**Why it matters:** + +- Advice bodies and span lifecycle methods run on the application (foreground) thread, where added cost is + customer-visible latency, as opposed to a background thread the tracer owns and paces itself +- Code meant to run only on a background thread must never become reachable from a foreground call site + +**What to do:** + +Mark code with `datadog.trace.api.function.ForegroundSafe` (cheap enough for either thread) or `.BackgroundOnly` +(must never be reached from the foreground) where the distinction matters, and see their Javadoc for the full +contract, including the inheritance-narrowing rule for overrides. This is a documentation-and-tooling convention +today (not yet compiler- or runtime-enforced); it is checked by hand and by the `dd-apm-sdk-review` skill's +performance lens. diff --git a/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java b/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java index a8f5fc5f3fa..3f832874b5c 100644 --- a/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java +++ b/internal-api/src/main/java/datadog/trace/api/cache/FixedSizeCache.java @@ -1,6 +1,7 @@ package datadog.trace.api.cache; import datadog.trace.api.Pair; +import datadog.trace.api.function.ForegroundSafe; import java.util.Arrays; import java.util.function.BiConsumer; import java.util.function.Function; @@ -16,9 +17,18 @@ * computeIfAbsent is idempotent, or otherwise you might not get back the value you expect * from a cache lookup. * + *

{@link ForegroundSafe}: the cache's own bookkeeping -- {@code computeIfAbsent}'s probing, + * {@code clear()}, {@code visit()}'s traversal -- always does a small, bounded amount of work + * against a fixed-size array: no growth, no eviction sweep. This guarantee covers only that + * bookkeeping, not the caller-supplied {@code producer} passed to {@code computeIfAbsent} or the + * {@code consumer} passed to {@code visit()} -- their cost is the caller's responsibility, exactly + * as with any higher-order method; a slow or blocking producer/consumer is not made safe by this + * annotation. + * * @param key type * @param value type */ +@ForegroundSafe abstract class FixedSizeCache implements DDCache { static final int MAXIMUM_CAPACITY = 1 << 30; diff --git a/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java b/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java new file mode 100644 index 00000000000..c20e80df70c --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/function/BackgroundOnly.java @@ -0,0 +1,82 @@ +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 code that must be confined to a background thread the tracer owns and paces itself -- e.g. + * serialization, stats aggregation, or eviction -- and must never be reached from an application + * thread (the foreground; see {@link ForegroundSafe}), where its cost would become customer-visible + * latency instead. + * + *

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 (see {@code APMLP-1645}) something to verify + * -- that no {@code @BackgroundOnly} code is reachable from a foreground call site. The discipline + * it names is not yet enforced; hold to it by hand until the checker lands. + * + *

The two markers are not symmetric -- see {@link ForegroundSafe} for why it, not this + * one, is the strictly stronger guarantee. + * + *

Orthogonal to {@code @ThreadSafe} (JSR-305): that annotation says concurrent calls from + * multiple callers are safe; this one says which category of caller is valid at all -- background + * threads (there may be more than one), never the foreground/application thread. A class can, and + * often will, carry both. + * + *

On a type ({@link ElementType#TYPE}): every method of this type is background-only + * unless a method-level {@link ForegroundSafe} widens it. + * + *

On a method ({@link ElementType#METHOD}): this method specifically is background-only, + * regardless of what the enclosing type declares -- a method-level marker always wins over the + * type-level one. + * + *

Inheritance direction. An override may only narrow a supertype's declared cost, never + * widen it -- the same variance rule as a covariant return type, applied to a cost contract instead + * of a value type. A method overriding a {@code @BackgroundOnly} supertype/interface method may + * itself be marked {@link ForegroundSafe} if that override happens to be cheap: every caller + * holding a reference typed to the supertype already assumed the worse (background-only) case, so a + * cheaper override can't surprise them. The reverse can't be done safely: overriding a {@link + * ForegroundSafe} or unannotated supertype method and marking the override {@code @BackgroundOnly} + * breaks the promise for every existing caller holding a supertype-typed reference, without their + * code changing at all -- this is a violation at the declaration site itself, independent of + * whether any foreground call site currently exists in the diff. + * + *

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 {@code dd-apm-sdk-review} skill's + * performance override, addendum J16) -- without needing to read this class's prose above. + * + *

+ */ +@Documented +@Retention(RetentionPolicy.CLASS) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface BackgroundOnly {} diff --git a/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java b/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java new file mode 100644 index 00000000000..71fe982d21c --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/function/ForegroundSafe.java @@ -0,0 +1,48 @@ +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 code cheap enough to call from an application thread (the foreground) -- the request or + * transaction thread the instrumented application itself is running, where any added cost is + * customer-visible latency, as opposed to a background thread the tracer owns and paces itself (see + * {@link BackgroundOnly}). + * + *

This is a documentation-and-tooling marker; it changes no behavior. It exists to telegraph the + * guarantee to readers and to give a future checker (see {@code APMLP-1645}) something to verify -- + * that no {@link BackgroundOnly} code is reachable from a foreground call site. The discipline it + * names is not yet enforced; hold to it by hand until the checker lands. + * + *

The two markers are not symmetric. {@code @ForegroundSafe} is the strictly stronger + * guarantee: code cheap enough for the foreground is automatically fine to call from a background + * thread too, so a {@code @ForegroundSafe} type or method may be called from either. {@link + * BackgroundOnly} code carries no such guarantee and must never be reached from a foreground call + * site. + * + *

On a type ({@link ElementType#TYPE}): every method of this type is foreground-safe + * unless a method-level {@link BackgroundOnly} narrows it. + * + *

On a method ({@link ElementType#METHOD}): this method specifically is foreground-safe, + * regardless of what the enclosing type declares -- a method-level marker always wins over the + * type-level one. + * + *

Inheritance direction: an override may narrow a {@link BackgroundOnly} supertype/ + * interface method to {@code @ForegroundSafe} (a cheaper override can't surprise a caller who + * already assumed the worse case) but may never widen a {@code @ForegroundSafe} or unannotated + * supertype method to {@link BackgroundOnly} -- see {@link BackgroundOnly}'s "Inheritance + * direction" for the full rule and why the reverse direction is a declaration-site violation on its + * own. + * + *

Checker contract. This annotation is not itself a trigger -- it is what makes a call + * site not suspect. See {@link BackgroundOnly}'s checker contract for the actual rule: + * code marked {@code @ForegroundSafe} (or carrying no marker) is exactly the caller side of the + * violation that contract flags when it reaches a {@code @BackgroundOnly} symbol. + */ +@Documented +@Retention(RetentionPolicy.CLASS) +@Target({ElementType.TYPE, ElementType.METHOD}) +public @interface ForegroundSafe {}