Skip to content

Commit a2eb6db

Browse files
dougqhdevflow.devflow-routing-intake
andauthored
Add @Staticlifetime and @singleton marker annotations (AI perf review) (#12646)
Add @Staticlifetime and @singleton marker annotations @Staticlifetime marks a field that must live for the whole program rather than for its enclosing instance's lifetime (e.g. a cache allocated per-request instead of once). @singleton is its escape valve, declaring a class has exactly one process-wide instance, so an instance field on it can satisfy @Staticlifetime without being static. Wires a matching check into the perf-review checks doc, mirroring the existing @NoEscape field-storage check. APMLP-1846, APMLP-1847 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Add Checker contract section to @singleton javadoc Splits narrative vs. machine-checkable rule, matching the convention established by @NoEscape and @Staticlifetime. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Annotate WebFlux route cache with @Staticlifetime Spot-checks the annotation against the real defect it was modeled on: PR #12561 (commit e6b8756) made this exact DDCache field static after it lived per-instance, unshared, for ~4 years. Serves as the first real call site for @Staticlifetime. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Annotate UTF8 tag/value caches with @Staticlifetime TAG_CACHE/VALUE_CACHE in TraceMapperV0_4 and KEY_CACHE/VALUE_CACHE in OtlpCommonProto are already static final -- marking them adds compliant-example coverage alongside RouteOnSuccessOrError's violation-turned-compliant case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Revert "Annotate WebFlux route cache with @Staticlifetime" Holding off on applying @Staticlifetime to real cache fields until @SuppressPerfContract (dougqh/perf-contract-suppress, PR #12647) lands -- want the exemption mechanism in place before widening the annotated surface. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Revert "Annotate UTF8 tag/value caches with @Staticlifetime" Same reason as the WebFlux route-cache revert: hold off until @SuppressPerfContract lands. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Close StaticLifetime checker-contract gaps: require final, static+non-final Per Bits AI review: the contract accepted "static" fields and singleton-scoped instance fields without requiring final, so a mutable field of either shape could pass while still being reassignable to a fresh, cold instance at runtime -- defeating the amortization guarantee the annotation exists to protect, just via a write instead of via scope. Add an explicit mutability trigger and require final in the Accepted description for both shapes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Fix StaticLifetime's ClassValue paragraph to match its own checker contract Per review: the intro implied a ClassValue-backed holder need not be static itself, contradicting the Accepted list below it, which requires "static final ClassValue<T>". Clarify that it's the per-Class values ClassValue computes that get a free process-wide lifetime, not the holder field referencing the ClassValue instance -- that still has to be static final. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Merge remote-tracking branch 'origin/master' into dougqh/static-lifetime-singleton # Conflicts: # .agents/skills/perf-review/references/checks.md Move StaticLifetime/Singleton into datadog.perfcontract and meta-annotate with @PerfContract Relocates both annotations out of the legacy datadog.trace.api.function package into the new datadog.perfcontract package (introduced by #12647), alongside PerfContract/SuppressPerfContract, and adds @PerfContract so tooling can discover them as perf-contract markers the same way it already discovers Strategy/StrategyConsumer/NoEscape. No other code references either annotation, so no import-site updates are needed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Tighten the @singleton Javadoc Applies a review suggestion: a static holder or DI registration does not by itself guarantee a single instance, and only final instance fields satisfy @Staticlifetime. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Restructure the @Staticlifetime Javadoc around one field rule Applies a review suggestion: a single accepted/violation/out-of-scope rule, an accurate ClassValue note, and block examples. Keeps the note on the name echoing Rust's 'static lifetime. Examples use <pre> with escaped @ and generics so javadoc does not read a leading @ as a block tag. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Require final for @Singleton-held fields in the perf-review StaticLifetime check Matches the @Staticlifetime and @singleton Javadoc: only a final instance field on a @singleton class is accepted, and ClassValue holders follow the plain static final rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent 14f69f0 commit a2eb6db

3 files changed

Lines changed: 97 additions & 2 deletions

File tree

‎.agents/skills/perf-review/references/checks.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,15 @@ Format: **pattern** — *expensive when (the interprocedural condition to trace)
2626
6. **FFI / native-boundary crossing on a hot path** *(central to the shared-core effort)* — *a native crossing per-span/per-item (not batched), or transporting strings/objects rather than primitives/IDs* — flag-with-confidence (boundary cost is mechanism-determined; runtime-specific pinning → addendum) — SEV-1/2 (SEV-1 if it blocks/pins under concurrency) — fix: batch (one per flush, not per item); transport interned IDs not strings; keep crossings off the hot/concurrency path.
2727
7. **Escape / allocation-elision defeated** *(Java/Go/.NET/V8 all have a version)* — *a refactor makes a previously-local object escape (stored, returned, captured by a closure, passed to a virtual/non-inlined call) → silent heap allocation on a hot path* — **flag-as-measure** ("may now escape and allocate; verify with an allocation profiler") — SEV-2/3 — fix: keep it local; avoid the escaping store/capture.
2828
8. **`@NoEscape` field-storage violation** — *a field (instance or static, directly or as a generic type argument) declared with an `@NoEscape`-annotated type (`datadog.trace.api.function.NoEscape`), **or** initialized directly from a call to an `@NoEscape`-annotated method — with no comment at the declaration justifying the retention*. The type form marks a concrete type (e.g. `SubSequence`); the method form exists for a return value whose concrete type can't itself carry the annotation (an anonymous class or lambda implementing a JDK interface). The annotation's own javadoc carries a self-contained "Checker contract" section (trigger / not-a-trigger / violation example / compliant example, both forms) 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 — compliant if the declaration carries either a `// Retained on purpose: <reason>`-style comment, or `@SuppressPerfContract(value = NoEscape.class, reason = "...")` (`datadog.perfcontract.SuppressPerfContract` — the generic exemption mechanism shared by every perf-contract marker, not `@NoEscape`-specific), or a named canned-exception annotation itself meta-annotated `@SuppressPerfContract` with `NoEscape.class` among its `value` (e.g. a hypothetical `@Borrowed`) — treat the meta-annotation's own `reason` as satisfying the requirement. Don't flag a `SuppressPerfContract`/canned-exception use as non-compliant just for being terse; that mirrors the annotation's own Checker contract. **flag-with-confidence** — SEV-2/3 (SEV-1 if the annotated type/method shares backing storage with something large, per its own javadoc). Current wearers: `SubSequence`, `Maybe` (type form; see J7 below for `SubSequence`'s specific retention-vs-transient discriminator). **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands and it can migrate down.**
29+
9. **`@StaticLifetime` field-lifetime violation** — *a field annotated `@StaticLifetime` (`datadog.perfcontract.StaticLifetime`) that is neither `static final` nor a `final` instance field declared on a class annotated `@Singleton` (`datadog.perfcontract.Singleton`); a shared `ClassValue` holder follows the same rule*. Declaration-scan only, same shape as the `@NoEscape` field-storage check above — it does not attempt to prove a field is actually reused enough to be worth caching, and it does not catch an unannotated per-instance cache; it only guards fields that opt in. `@Singleton`, applied to a class, is a trusted, unverified declaration (v1 posture — the same stance the other perf-contract markers take toward `static final` itself) that the class has exactly one process-wide instance; declaring it satisfies `@StaticLifetime` for that class's `final` instance fields. **flag-with-confidence** — SEV-2/3, motivated by a real production defect (a per-request-constructed cache that never actually amortized anything — see the annotation's own javadoc for the full trigger/accepted-form table). Refines the "per-call cache / expensive-object creation that should be static/once" deterministic-lint candidate below by giving that pattern's opted-in subset an explicit annotation and trigger; the unannotated general case is still that line's job. **No lint enforces this yet — the AI reviewer is the only check, so this stays here (not under deterministic-lint candidates below) until a checker lands and it can migrate down.**
2930

3031
## Deterministic-lint candidates (DON'T spend AI budget — make these real lints)
3132
Fixed-signature, mechanically checkable:
32-
- per-call cache / regex / expensive-object creation that should be static/once
33+
- per-call cache / regex / expensive-object creation that should be static/once (the `@StaticLifetime`-annotated subset is checked above; this line covers the still-unannotated general case)
3334
- boxing in specific hot APIs
3435
- using a string-API where an id-API exists on a hot decorator
3536
- the existing convention rules (e.g. don't extract one-shot instrumentation methods to constants)
36-
- *(grows as patterns prove mechanically checkable — migrate them off the AI as they stabilize; the `@NoEscape` field-storage check above belongs here once its checker lands)*
37+
- *(grows as patterns prove mechanically checkable — migrate them off the AI as they stabilize; the `@NoEscape` and `@StaticLifetime` field-storage checks above belong here once their checkers land)*
3738

3839
## Java addendum (JVM-specific — mechanism authored with JIT-developer authority; **calibrate production-priority against your own escalation history**)
3940
Refines the universal checks with JVM mechanics. Quarantined here, for the Java audience that has the substrate.
Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
package datadog.perfcontract;
2+
3+
import java.lang.annotation.Documented;
4+
import java.lang.annotation.ElementType;
5+
import java.lang.annotation.Retention;
6+
import java.lang.annotation.RetentionPolicy;
7+
import java.lang.annotation.Target;
8+
9+
/**
10+
* Declares that a class has one instance for the life of the process. All construction paths must
11+
* preserve that guarantee; a static holder or DI registration alone does not prevent another
12+
* instance from being created.
13+
*
14+
* <p>This marker changes no runtime behavior. Tools trust the declaration without checking
15+
* construction sites. If the class is instantiated more than once, they may accept caches that are
16+
* rebuilt with each instance.
17+
*
18+
* <p><b>Checker contract.</b> This annotation has no violation rule of its own. It allows {@link
19+
* StaticLifetime}'s checker to accept {@code final} instance fields declared on the annotated
20+
* class.
21+
*/
22+
@Documented
23+
@PerfContract
24+
@Retention(RetentionPolicy.CLASS)
25+
@Target(ElementType.TYPE)
26+
public @interface Singleton {}
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
package datadog.perfcontract;
2+
3+
import java.lang.annotation.Documented;
4+
import java.lang.annotation.ElementType;
5+
import java.lang.annotation.Retention;
6+
import java.lang.annotation.RetentionPolicy;
7+
import java.lang.annotation.Target;
8+
9+
/**
10+
* Marks a field whose value must be retained for the life of the program to share its setup cost
11+
* across uses. A cache rebuilt with each request repeats that cost.
12+
*
13+
* <p>The name echoes Rust's {@code 'static} lifetime rather than the Java keyword {@code static}:
14+
* the property is "lives for the whole program," which a {@code final} field on a {@link Singleton}
15+
* satisfies without being {@code static}.
16+
*
17+
* <p>This marker changes no runtime behavior. The declaration rules below guide AI review; no
18+
* static checker currently enforces them.
19+
*
20+
* <p><b>Checker contract.</b> Inspect only fields annotated {@code @StaticLifetime}:
21+
*
22+
* <ul>
23+
* <li><b>Accepted:</b> a {@code static final} field, or a {@code final} instance field declared
24+
* on a class annotated {@link Singleton}. The singleton declaration is trusted, not verified.
25+
* <li><b>Violation:</b> any other annotated field. This includes fields without {@code final} and
26+
* instance fields on classes without {@code @Singleton}, even if a class is constructed only
27+
* once per session.
28+
* <li><b>Out of scope:</b> unannotated fields, how often a value is reused, and replacement of
29+
* state inside the referenced object. The check does not follow references through the object
30+
* graph.
31+
* </ul>
32+
*
33+
* <p>Requiring {@code final} prevents reassignment from discarding cached state and repeating
34+
* setup. It does not prevent the referenced object from changing its own state.
35+
*
36+
* <p>A shared {@link java.lang.ClassValue} satisfies the same field rules. Its values are cached
37+
* per class; computation may be repeated under races or after {@link
38+
* java.lang.ClassValue#remove(Class) remove}. This contract covers the shared holder, not the
39+
* lifetime of each cached value.
40+
*
41+
* <p>Violation examples: an instance cache on a class without {@code @Singleton}, or a static cache
42+
* without {@code final}.
43+
*
44+
* <pre>
45+
* &#64;StaticLifetime
46+
* private final DDCache&lt;String, String&gt; instanceCache = DDCaches.newFixedSizeCache(128);
47+
*
48+
* &#64;StaticLifetime
49+
* private static DDCache&lt;String, String&gt; staticCache = DDCaches.newFixedSizeCache(128);
50+
* </pre>
51+
*
52+
* <p>Compliant examples:
53+
*
54+
* <pre>
55+
* &#64;StaticLifetime
56+
* private static final DDCache&lt;String, String&gt; CACHE = DDCaches.newFixedSizeCache(128);
57+
*
58+
* &#64;Singleton class Registry {
59+
* &#64;StaticLifetime
60+
* private final DDCache&lt;String, String&gt; cache = DDCaches.newFixedSizeCache(128);
61+
* }
62+
* </pre>
63+
*/
64+
@Documented
65+
@PerfContract
66+
@Retention(RetentionPolicy.CLASS)
67+
@Target(ElementType.FIELD)
68+
public @interface StaticLifetime {}

0 commit comments

Comments
 (0)