From ec25e739f7e543c67d3e313f8d772ec39afb1c91 Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 18:32:30 -0400 Subject: [PATCH 01/12] =?UTF-8?q?performance-notes:=20record=20String?= =?UTF-8?q?=E2=86=92Name=20annotation-name=20migration=20as=20flat?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document the measured-flat result for migrating annotation-name dispatch from value-based String comparison (AnnotationUtils.annotationName) to javac Name identity comparison (annotationNameAsName + ==), framework-wide. The fully-realized form (CFAM caches its Name, annotationNameAsName has a CFAM fast path, isSupportedQualifier uses a companion Set) is within the run-to-run noise band on both allocation and wall clock across nullness and Value workloads. Root cause: CheckerFrameworkAnnotationMirror already caches the decoded interned name, so the toString()/intern() the change targets is not on the hot path. A ceiling profile on an annotation-saturated workload puts all name-handling frames at 0 samples; the hot leaf that does appear (IdentityHashMap.get -> getQualifierKind) is itself already rejected. Also add the "measure the ceiling on an adversarial workload" habit and a do-not-propose entry to the cf-performance skill. Co-Authored-By: Claude Opus 4.8 --- .claude/skills/cf-performance/SKILL.md | 20 ++++++++++ docs/developer/performance-notes.md | 55 ++++++++++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/.claude/skills/cf-performance/SKILL.md b/.claude/skills/cf-performance/SKILL.md index ede5ec073daf..d0e8eaedeb27 100644 --- a/.claude/skills/cf-performance/SKILL.md +++ b/.claude/skills/cf-performance/SKILL.md @@ -376,6 +376,20 @@ but cost ≈10% wall clock (far more than its hit-rate delta implied). Cut guard is a counter on the code you changed — if it never fires, "no effect" is a workload bug, not a verdict on the change. Pair this with picking a workload that *maximizes* traffic through the target (the size-sweep / shape generators exist for this). +- **Measure the *ceiling* before micro-optimizing a dispatch — profile the adversarial + workload, not the typical one.** When tempted to change *how* something is represented or + compared (string vs. `Name`, a "kind" tag, a second-level cache), first bound the maximum + possible win: build a workload that *maximizes* traffic through the target and check whether the + target's frames even appear above the sample floor. If they don't, no variant can help and you + stop in one profile instead of an A/B per variant. The String→`Name` annotation-name migration + (see *Tried and rejected*) died this way: an annotation-saturated, checker-bound workload (400 + methods × 40 explicitly-annotated locals) at 2596 samples showed `annotationName` / + `annotationNameAsName` / `areSameByName` / `isSupportedQualifier` / `Name.toString` / `intern` + at **0 samples** — and the hot leaf that *did* surface (`IdentityHashMap.get` → `getQualifierKind`) + was itself an already-rejected target. The reason the ceiling was ~0: the hot representation + (`CheckerFrameworkAnnotationMirror`) already caches the decoded interned name, so the decode the + change "removed" was never happening. **Before optimizing a lookup, ask what the common carrier + already caches.** ## Removing a defensive copy (the opposite of adding a cache) @@ -499,6 +513,12 @@ In order of historical hit rate on this codebase: - Switching map implementations purely on micro-benchmark intuition — measure on a real workload. - Lock-removal changes without auditing all reachable threads. +- Rerepresenting annotation *names* for dispatch (`String`→`Name` + `==`, + per-qualifier "kind" tags) or caching `getQualifierKind` — both measured + flat (see *Tried and rejected*). `CheckerFrameworkAnnotationMirror` + already caches the decoded interned name, so the decode you would remove + is not on the hot path; identity-comparing `Name`s is no cheaper than the + existing interned-`String` comparison. ## Producing the patch diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index fb6dd261a225..90f30555bee6 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -1874,6 +1874,61 @@ the prior finding. A fresh hypothesis is not new evidence. only sanctioned rescue in this basin is a maintained incremental content-hash on `CFAbstractStore` (see the Short list and the rejected equal-store merge short-circuit above), memory-A/B-gated. +- **`String`→`Name` annotation-name migration: replace `AnnotationUtils.annotationName` (String) + with `annotationNameAsName` (javac `Name`) + `==` identity comparison, framework-wide (June + 2026).** Branch converting the name-dispatch sites in `ValueAnnotatedTypeFactory`, + `ValueQualifierHierarchy`, `ValueTransfer`, the Index `UpperBound*` checkers, `Units`, + `BaseTypeVisitor`/`BaseTypeValidator` (`qualAllowedLocations`), `DependentTypesHelper` + (`annoToElements`), and `AnnotatedTypeFactory` (`aliases`, `declAliases`, `isSupportedQualifier`) + from value-based `String` comparison to `Name`-identity comparison, backing maps switched to + `IdentityHashMap`. Premise: "avoid the `Name.toString()` decode + `String.intern()` that + `annotationName` performs." + + **Measured flat — alloc and wall — even after completing the half-finished form.** As proposed the + branch slightly *regresses*, because `annotationNameAsName` had no + `CheckerFrameworkAnnotationMirror` (CFAM) fast path (it pointer-chases + `getAnnotationType().asElement().getQualifiedName()` where `annotationName` reads a cached field) + and `isSupportedQualifier(Name)` still called `name.toString()` on every cache miss. To measure the + *approach* and not the artifacts, the branch was completed: cache a `Name` field on CFAM next to its + existing interned-`String` `annotationName`; give `annotationNameAsName` a CFAM fast path; replace + `isSupportedQualifier`'s `getSupportedTypeQualifierNames().contains(name.toString())` with a + one-time identity-backed companion `Set` membership test. A/B of that fully-realized version + vs. master (deterministic `jdk.ThreadAllocationStatistics`, single forked `javac`): + + | corpus | metric | master | realized | delta | + | --- | --- | --- | --- | --- | + | nullness, `inherit` shape (~7.4k LoC) | alloc (median of 5) | 4488.0 MB | 4492.3 MB | +0.10% (noise) | + | nullness, `repeat` shape (~7.3k LoC) | alloc | 785.7 MB | 787.5 MB | +0.23% (noise) | + | Value, 8 test inputs batched | alloc | 312.6 MB | 312.9 MB | +0.10% (noise) | + | nullness, `inherit` | wall (2nd-best of 4) | 7.98 s | 8.18 s | noise | + | nullness, `repeat` | wall | 5.10 s | 5.05 s | noise | + + **Root cause — the targeted allocation does not happen on hot paths.** CFAM, the representation the + framework manipulates for the overwhelming majority of annotations, already caches its name as an + `@Interned String` computed once in its constructor, so `annotationName(am)` was *already a field + read with zero allocation* for CFAM. The `toString().intern()` cost applies only to raw + `Attribute.Compound` source mirrors, and even there `getAnnotationType().asElement()` is two field + reads (`anno.type.tsym`). You cannot remove garbage that is not being produced. + + **Ceiling proof.** A deliberately annotation-saturated, checker-bound workload (400 methods × 40 + explicitly-`@Nullable`/`@NonNull`/`@MonotonicNonNull` locals each), profiled at 2596 + `ExecutionSample`s, shows the name-handling frames — `annotationName`, `annotationNameAsName`, + `areSameByName`, `isSupportedQualifier`, `Name.toString`, `String.intern` — **entirely below the + sample floor (0 samples).** The hot leaf there is `IdentityHashMap.get` (7.8%), attributed + (`jfr-analyze self`) ~40% to `ElementQualifierHierarchy.getQualifierKind` — annotation→`QualifierKind` + resolution, *not* name handling — and caching *that* is itself already measured-and-rejected (see + "`AnnotationMirror → QualifierKind` second-level cache" above). **General lesson: annotation-name + comparison/dispatch is not a hotspot; CFAM already caches the decoded interned name, so any + String→`Name` rerepresentation is flat. This is the dispatch-level analogue of the `isStringEqual` + false premise (see "Element and name caching"). The annotation-name allocation that *does* show up + (`[B` UTF-8 decode, ~28% of dataflow allocation) is `Name.toString()` deep in dataflow/store + machinery, not in qualifier-name dispatch — optimize there, not here.** The branch also seeded three + anti-patterns worth recording: a `static IdentityHashMap` (Names are interned *per + compilation context*, so a static identity-map both leaks across compilation tasks and never hits + cross-context — keep such caches instance-scoped), unused cached-`Name` fields hidden behind a + class-wide `@SuppressWarnings("UnusedVariable")`, and `Name`-identity assumptions spread across the + public `javacutil` surface for no measured gain. + --- ## Short list From 128002ea8be0964a316d78c221ba0d1087cc655e Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 19:24:30 -0400 Subject: [PATCH 02/12] performance-notes: reject pre-flow and phase-scoped getAnnotatedType caches Replace the placeholder "scope-bounded expression-type cache" direction (#2 under the architectural getAnnotatedType item) with the measured result. Instrumented addComputedTypeAnnotations(Tree, ATM), splitting it at the useFlow boundary and recording per-tree redundancy plus the unsound fraction on all-systems (120 files) and a loop-heavy workload. Both forms are unsound. Caching the pre-flow (2a) type is 25% (all-systems) to 38% (loops) unstable because tree annotators transitively query flow-refined subexpression types. A phase-scoped full cache, restricted to the post-flow checking traversal (analysis.isRunning() false, frozen flowResult), is still 26% to 59% unstable: the same expression tree yields different types across repeated queries even after flow completes. The flow step itself is cheap, so the cost lives in the 2a computation whose result is the unstable thing; the stable trees are the cheap-to-recompute ones. Co-Authored-By: Claude Opus 4.8 --- docs/developer/performance-notes.md | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index 90f30555bee6..508965cdaac7 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -2066,7 +2066,31 @@ Capture format: hot method, hypothesis, blockers. the soundness crux flagged here. And the instability is *highest where the redundancy is highest* (loops). A per-analysis memo would cache stale/wrong types; a per-*iteration* memo would be sound but the within-iteration redundancy is ~0. Low value and unsound — do not pursue. - 2. **Scope-bounded expression-type cache** (per visitor-subtree) — broader, same soundness crux. + 2. **Scope-bounded / pre-flow expression-type cache — MEASURED AND REJECTED (June 2026).** Two + forms tested by instrumenting `GenericAnnotatedTypeFactory.addComputedTypeAnnotations(Tree, + ATM)`, splitting it at the `if (this.useFlow)` boundary into the flow-independent prefix (2a: + `applyQualifierParameterDefaults` → tree/type annotators → `defaults.annotate`) and the + flow-dependent suffix (2b: `getInferredValueFor` + `applyInferredAnnotations`), with per-tree + `IdentityHashMap` signature maps recording redundancy and the *unsound fraction* + (= repeats whose computed type differs from the prior result for the same tree). Single forked + `javac -processor nullness`, on all-systems (120 files) and a loop-heavy artificial workload. + (a) **Split-cache (cache the *pre-flow* 2a type, always recompute 2b) — unsound.** Redundancy is + high (repeats 91% all-systems / 86% loops of expression calls) but the pre-flow type is + **unstable 25% (all-systems) / 38% (loops)** of repeats, because the "flow-independent" tree + annotators transitively call `getAnnotatedType` on subexpressions whose types are flow-refined. + (b) **Phase-scoped full cache (cache the full type, but only when `analysis.isRunning()` is + false — i.e. the post-flow checking traversal with a frozen `flowResult`) — also unsound.** + Even restricted to the checking phase the full type is **unstable 26% (all-systems) / 59% + (loops)** of repeats (CHK repeats 91% / 68%). The same expression tree genuinely yields + different types across repeated queries even after flow analysis completes, so a + tree-identity-keyed cache would serve a stale/wrong type a quarter to a half of the time — worse + than the per-analysis `getValueFromFactory` memo (#1, 8–18%) already rejected. Cost context + confirms it is not even worth chasing the sound subset: the flow step 2b is cheap (~0.3 s + inclusive vs. a multi-second 2a), so the expense is entirely in the 2a computation **whose + result is the unstable thing** — and the stable trees (literals, simple identifiers) are exactly + the cheap-to-recompute ones (the "count ≠ cost" / #4 dead end). No safe getAnnotatedType-level + expression cache exists; the sound caches (skeleton `fromExpressionTreeCache`, + `methodAsMemberOfCache`, `classAndMethodTreeCache`) are already in place. Do not pursue. 3. **Split flow-independent structure from flow-dependent annotations — MEASURED, ALREADY IMPLEMENTED (June 2026).** The hypothesis was: `fromExpression` (~24% inclusive) builds a deterministic-per-tree skeleton, so cache the frozen skeleton and re-apply only the From 8f46410e6f03af833ee65be8dbe0174d788e7dc0 Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 19:36:40 -0400 Subject: [PATCH 03/12] performance-notes: reject leaf-subset getAnnotatedType prefix cache Extend the rejected getAnnotatedType expression-cache direction with the leaf-subset measurement. The hypothesis was that subexpression-free leaves (identifiers, literals) have a flow-independent prefix and so could be cached even where compound trees cannot. Instrumented, split by kind, checking-phase only. Rejected: in the checking phase the pre-flow leaf type is still unstable 22% (identifiers) / 40% (literals) on all-systems. The cause is context- dependence in which hierarchy's annotations are applied -- the same `1` literal returns `int` vs `@Initialized int` (nullness runs with the Initialization checker) -- so a tree-keyed cache would serve an under-annotated type. The cost is immaterial anyway: safely-cacheable checking-phase leaf 2a is ~1.5% of compile. getAnnotatedType is keyed by call context, not tree identity. Co-Authored-By: Claude Opus 4.8 --- docs/developer/performance-notes.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index 508965cdaac7..cffadbdcfeb3 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -2091,6 +2091,20 @@ Capture format: hot method, hypothesis, blockers. the cheap-to-recompute ones (the "count ≠ cost" / #4 dead end). No safe getAnnotatedType-level expression cache exists; the sound caches (skeleton `fromExpressionTreeCache`, `methodAsMemberOfCache`, `classAndMethodTreeCache`) are already in place. Do not pursue. + (c) **Even the subexpression-free *leaf* subset (identifiers/literals), checking-phase only — + unsound, and immaterial.** The natural rescue ("the instability comes from tree annotators + querying flow-refined subexpressions, so restrict the cache to leaves that have none") was + measured: in the checking phase the pre-flow leaf type is still **unstable 22% (identifiers) / + 40% (literals)** on all-systems, 58% / 43% on loops. The cause is *not* subexpression flow — it + is that the computed type is context-dependent in *which hierarchy's annotations are applied*: + the same `1` literal returns `int` in one query and `@Initialized int` in another (nullness runs + with the Initialization checker), so a tree-keyed cache would hand back an under-annotated type + and break subtyping. Independently, the cost ceiling is immaterial: the safely-cacheable + checking-phase leaf 2a self-time is only ~0.22 s (~1.5% of a 13.9 s 120-file compile); the large + leaf-2a figure (18× the deepCopy cost) is dominated by *dataflow-phase* leaf work, which is not + safe to cache. **Lesson: `getAnnotatedType`'s result is keyed by call *context*, not by tree + identity — even for a literal — so no tree-identity cache at this level is sound, at any + granularity.** 3. **Split flow-independent structure from flow-dependent annotations — MEASURED, ALREADY IMPLEMENTED (June 2026).** The hypothesis was: `fromExpression` (~24% inclusive) builds a deterministic-per-tree skeleton, so cache the frozen skeleton and re-apply only the From 8267ae006c7c4ce6f8374a963d91f8dc9383c42a Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 19:58:35 -0400 Subject: [PATCH 04/12] performance-notes: correct getAnnotatedType pre-flow cache rejection My two earlier entries rejected the pre-flow ("split") and leaf-subset getAnnotatedType caches as "25-59% unstable." That was a measurement artifact: the per-tree signature used AnnotatedTypeMirror.toString(), which conflates cross-hierarchy completeness (int vs @Initialized int = the Initialization annotation absent vs present) with real within-hierarchy disagreement, and the comparison indexed getTopAnnotations() (an unstable- order Set) positionally, misaligning hierarchies across calls. Re-measured per hierarchy (keyed by top-annotation name; present-disagree and completeness counted separately): value leaves are 0% unstable / 0% incomplete over ~70k (all-systems) and ~208k (loops) repeats; compound expressions ~0.46% / 0%; type-name identifiers ~0.7%. The pre-flow type is stable; flow-dependence is confined to step 2b, as the split assumed. So the split cache is a sound candidate (value still pending a built-cache A/B). Reclassify accordingly and record the per-hierarchy measurement method in the skill so the toString trap is not repeated. Co-Authored-By: Claude Opus 4.8 --- .claude/skills/cf-performance/SKILL.md | 12 ++++ docs/developer/performance-notes.md | 82 ++++++++++++++------------ 2 files changed, 55 insertions(+), 39 deletions(-) diff --git a/.claude/skills/cf-performance/SKILL.md b/.claude/skills/cf-performance/SKILL.md index d0e8eaedeb27..314ef8816035 100644 --- a/.claude/skills/cf-performance/SKILL.md +++ b/.claude/skills/cf-performance/SKILL.md @@ -390,6 +390,18 @@ but cost ≈10% wall clock (far more than its hit-rate delta implied). Cut (`CheckerFrameworkAnnotationMirror`) already caches the decoded interned name, so the decode the change "removed" was never happening. **Before optimizing a lookup, ask what the common carrier already caches.** +- **To decide if a per-tree type can be cached, compare its annotations PER HIERARCHY — never via + `AnnotatedTypeMirror.toString()`.** `toString()` over-reports "instability" by conflating (i) + cross-hierarchy *completeness* (an `int` literal printing as `int` vs `@Initialized int` is the + Initialization hierarchy's annotation absent vs present — not a within-hierarchy change; harmless + to a per-hierarchy cache) with real within-hierarchy disagreement. And **never index a qualifier + `Set` (`getTopAnnotations()`) positionally across calls** — its iteration order is unstable, so + slot *i* silently compares different hierarchies. This trap produced a *wrong, committed* + conclusion this session: the pre-flow `getAnnotatedType` split-cache was rejected as "25–59% + unstable" on a `toString()` signature; re-measuring per hierarchy (keyed by top-annotation name, + splitting present-disagree from completeness) showed value-leaf pre-flow types are **0%** unstable + and compound ~0.46% — reopening it as a candidate (see *Tried and rejected* → getAnnotatedType #2). + The right signature is a map {hierarchy → annotation-in-that-hierarchy}, compared key-by-key. ## Removing a defensive copy (the opposite of adding a cache) diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index cffadbdcfeb3..48e12fe5a16f 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -2066,45 +2066,49 @@ Capture format: hot method, hypothesis, blockers. the soundness crux flagged here. And the instability is *highest where the redundancy is highest* (loops). A per-analysis memo would cache stale/wrong types; a per-*iteration* memo would be sound but the within-iteration redundancy is ~0. Low value and unsound — do not pursue. - 2. **Scope-bounded / pre-flow expression-type cache — MEASURED AND REJECTED (June 2026).** Two - forms tested by instrumenting `GenericAnnotatedTypeFactory.addComputedTypeAnnotations(Tree, - ATM)`, splitting it at the `if (this.useFlow)` boundary into the flow-independent prefix (2a: - `applyQualifierParameterDefaults` → tree/type annotators → `defaults.annotate`) and the - flow-dependent suffix (2b: `getInferredValueFor` + `applyInferredAnnotations`), with per-tree - `IdentityHashMap` signature maps recording redundancy and the *unsound fraction* - (= repeats whose computed type differs from the prior result for the same tree). Single forked - `javac -processor nullness`, on all-systems (120 files) and a loop-heavy artificial workload. - (a) **Split-cache (cache the *pre-flow* 2a type, always recompute 2b) — unsound.** Redundancy is - high (repeats 91% all-systems / 86% loops of expression calls) but the pre-flow type is - **unstable 25% (all-systems) / 38% (loops)** of repeats, because the "flow-independent" tree - annotators transitively call `getAnnotatedType` on subexpressions whose types are flow-refined. - (b) **Phase-scoped full cache (cache the full type, but only when `analysis.isRunning()` is - false — i.e. the post-flow checking traversal with a frozen `flowResult`) — also unsound.** - Even restricted to the checking phase the full type is **unstable 26% (all-systems) / 59% - (loops)** of repeats (CHK repeats 91% / 68%). The same expression tree genuinely yields - different types across repeated queries even after flow analysis completes, so a - tree-identity-keyed cache would serve a stale/wrong type a quarter to a half of the time — worse - than the per-analysis `getValueFromFactory` memo (#1, 8–18%) already rejected. Cost context - confirms it is not even worth chasing the sound subset: the flow step 2b is cheap (~0.3 s - inclusive vs. a multi-second 2a), so the expense is entirely in the 2a computation **whose - result is the unstable thing** — and the stable trees (literals, simple identifiers) are exactly - the cheap-to-recompute ones (the "count ≠ cost" / #4 dead end). No safe getAnnotatedType-level - expression cache exists; the sound caches (skeleton `fromExpressionTreeCache`, - `methodAsMemberOfCache`, `classAndMethodTreeCache`) are already in place. Do not pursue. - (c) **Even the subexpression-free *leaf* subset (identifiers/literals), checking-phase only — - unsound, and immaterial.** The natural rescue ("the instability comes from tree annotators - querying flow-refined subexpressions, so restrict the cache to leaves that have none") was - measured: in the checking phase the pre-flow leaf type is still **unstable 22% (identifiers) / - 40% (literals)** on all-systems, 58% / 43% on loops. The cause is *not* subexpression flow — it - is that the computed type is context-dependent in *which hierarchy's annotations are applied*: - the same `1` literal returns `int` in one query and `@Initialized int` in another (nullness runs - with the Initialization checker), so a tree-keyed cache would hand back an under-annotated type - and break subtyping. Independently, the cost ceiling is immaterial: the safely-cacheable - checking-phase leaf 2a self-time is only ~0.22 s (~1.5% of a 13.9 s 120-file compile); the large - leaf-2a figure (18× the deepCopy cost) is dominated by *dataflow-phase* leaf work, which is not - safe to cache. **Lesson: `getAnnotatedType`'s result is keyed by call *context*, not by tree - identity — even for a literal — so no tree-identity cache at this level is sound, at any - granularity.** + 2. **Pre-flow expression-type ("split") cache — RE-MEASURED; a genuine candidate, value pending an + A/B (June 2026).** Split `addComputedTypeAnnotations(Tree, ATM)` at the `if (this.useFlow)` + boundary into the prefix 2a (`applyQualifierParameterDefaults` → tree/type annotators → + `defaults.annotate`) and the flow suffix 2b (`getInferredValueFor` + `applyInferredAnnotations`). + Hypothesis: cache the *pre-flow* (post-2a) type per tree and recompute only 2b, since 2b is where + flow lives. + + **Correction to an earlier rejection.** A first pass keyed the per-tree signature on + `AnnotatedTypeMirror.toString()` and concluded the pre-flow type was "unstable 25–40% (leaves) / + 26–59% (compound, even checking-phase)," i.e. that no tree-keyed cache could be sound. **That was + a measurement artifact, not a property of the code.** `toString()` conflates two non-issues with + real instability: (i) *cross-hierarchy completeness* — a primitive literal printing as `int` vs + `@Initialized int` is the Initialization hierarchy's annotation absent vs present, not a + within-hierarchy disagreement; and (ii) the comparison read `getTopAnnotations()` (a `Set` with + unstable iteration order) *positionally*, so slot *i* compared different hierarchies across calls. + Re-measured *per hierarchy* (keyed by top-annotation name; counting "present-disagree" = same + hierarchy, two different non-∅ annotations, and "completeness" = ∅-vs-present, separately): + + | category | all-systems (120f) | loops | + | --- | --- | --- | + | value leaf (literal / var / field / param id) | disagree 0, compl 0 / 69,557 repeats | 0 / 0 / 207,940 | + | compound expression | disagree 211 (0.46%), compl 0 / 46,166 | 0 / 0 / 57,928 | + | type-name identifier (class/enum) | 86 (0.7%) | 0 | + + So per hierarchy the pre-flow type is **stable**: value leaves never disagree, compound + expressions disagree ~0.46% (the genuinely context-dependent residual — `NewClass`/`NewArray`/ + conditionals, the #602 set in #3 below), type-name identifiers ~0.7% (excludable by element + kind). The flow-dependence is entirely in 2b, exactly as the split assumed. **This reopens the + split cache as a *sound* candidate** — provided it returns a deep copy (callers mutate) and skips + the ~0.5–0.7% context-dependent kinds. + + **Value is NOT yet confirmed; do an implemented-cache A/B before adoption.** Instrumented 2a + self-time for value leaves alone is ~0.55 s (~4% of a 12.6 s all-systems compile) and ~1.16 s + (~15%) on loop-heavy code, vs ~0.05–0.17 s deepCopy — a favorable projected ratio, and compound + 2a (not separately timed) is larger. But projected savings have repeatedly evaporated in A/B this + session (the String→`Name` flat result; #4 applied-defaults). The next step is to *build* the + post-2a cache (store the type just before the `useFlow` block; on hit `deepCopy` + run only 2b; + key by `Tree`; clear per CU like the other tree caches; skip type-name identifiers and the #602 + compound kinds) and measure deterministic allocation + wall on all-systems and loops. Until that + A/B exists this is a *candidate*, not a win. (The companion "phase-scoped full cache" idea was + rejected on the same flawed `toString()` measure and should likewise be re-measured per hierarchy + if revisited.) **Method lesson: never measure annotated-type stability via `toString()` — compare + per hierarchy, and never index a qualifier `Set` positionally across calls.** 3. **Split flow-independent structure from flow-dependent annotations — MEASURED, ALREADY IMPLEMENTED (June 2026).** The hypothesis was: `fromExpression` (~24% inclusive) builds a deterministic-per-tree skeleton, so cache the frozen skeleton and re-apply only the From f35e83829661d05d5f42553e9c09996d4adf9365 Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 21:12:39 -0400 Subject: [PATCH 05/12] performance-notes: fix root-cause of getAnnotatedType cache mis-measurement My prior correction reached the right conclusion (the pre-flow split cache is a sound candidate) but blamed the wrong cause: "unstable getTopAnnotations iteration order." That is wrong. getTopAnnotations returns a build-once, ArrayList-backed AnnotationMirrorSet over a TreeMap -- already a stable, deterministic order. The actual confound was the instrumentation: a static per-Tree map shared across the FOUR sub-factories a nullness compile runs (NullnessNoInit, Initialization, InitializationFieldAccess, KeyFor), so it compared one type system's snapshot of a tree against another's. Re-measured with a per-factory (instance) map: value leaves are still 0% disagree / 0% completeness (58,447 / 130,894 repeats); compound 0.69% / 0. The candidate stands; the explanation is now correct. Skill updated with both measurement traps (toString completeness conflation; static Tree-keyed state across factories). Co-Authored-By: Claude Opus 4.8 --- .claude/skills/cf-performance/SKILL.md | 27 ++++++------ docs/developer/performance-notes.md | 57 ++++++++++++++++---------- 2 files changed, 50 insertions(+), 34 deletions(-) diff --git a/.claude/skills/cf-performance/SKILL.md b/.claude/skills/cf-performance/SKILL.md index 314ef8816035..cdaef51b5dbb 100644 --- a/.claude/skills/cf-performance/SKILL.md +++ b/.claude/skills/cf-performance/SKILL.md @@ -390,18 +390,21 @@ but cost ≈10% wall clock (far more than its hit-rate delta implied). Cut (`CheckerFrameworkAnnotationMirror`) already caches the decoded interned name, so the decode the change "removed" was never happening. **Before optimizing a lookup, ask what the common carrier already caches.** -- **To decide if a per-tree type can be cached, compare its annotations PER HIERARCHY — never via - `AnnotatedTypeMirror.toString()`.** `toString()` over-reports "instability" by conflating (i) - cross-hierarchy *completeness* (an `int` literal printing as `int` vs `@Initialized int` is the - Initialization hierarchy's annotation absent vs present — not a within-hierarchy change; harmless - to a per-hierarchy cache) with real within-hierarchy disagreement. And **never index a qualifier - `Set` (`getTopAnnotations()`) positionally across calls** — its iteration order is unstable, so - slot *i* silently compares different hierarchies. This trap produced a *wrong, committed* - conclusion this session: the pre-flow `getAnnotatedType` split-cache was rejected as "25–59% - unstable" on a `toString()` signature; re-measuring per hierarchy (keyed by top-annotation name, - splitting present-disagree from completeness) showed value-leaf pre-flow types are **0%** unstable - and compound ~0.46% — reopening it as a candidate (see *Tried and rejected* → getAnnotatedType #2). - The right signature is a map {hierarchy → annotation-in-that-hierarchy}, compared key-by-key. +- **Two traps when measuring whether a per-tree type is stable enough to cache** (both produced a + *wrong, committed* conclusion this session — the pre-flow `getAnnotatedType` split-cache was + rejected as "25–59% unstable," then re-measuring correctly showed value leaves are **0%** unstable): + 1. **Compare PER HIERARCHY, never via `AnnotatedTypeMirror.toString()`.** `toString()` over-reports + instability by counting *cross-hierarchy completeness* — an `int` literal printing as `int` vs + `@Initialized int` is just the Initialization hierarchy's annotation absent vs present, harmless + to a per-hierarchy cache — as if it were a within-hierarchy disagreement. The right signature is + a map {hierarchy-top-name → annotation-in-that-hierarchy}, compared key-by-key. (`getTopAnnotations()` + order is *not* a hazard — it is a build-once `ArrayList`-backed `AnnotationMirrorSet` over a + `TreeMap`, already stable.) + 2. **A checker runs SEVERAL `AnnotatedTypeFactory` instances — never key shared/`static` state on + `Tree`.** A `nullness` compile drives four GATFs (`NullnessNoInit`, `Initialization`, + `InitializationFieldAccess`, `KeyFor`), each with its own qualifier hierarchy. A `static` + per-`Tree` map (or cache) mixes one type system's result for a tree with another's. Keep such + state on the factory instance; confirm by logging `this.getClass().getSimpleName()`. ## Removing a defensive copy (the opposite of adding a cache) diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index 48e12fe5a16f..e117ac76dcd9 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -2073,29 +2073,39 @@ Capture format: hot method, hypothesis, blockers. Hypothesis: cache the *pre-flow* (post-2a) type per tree and recompute only 2b, since 2b is where flow lives. - **Correction to an earlier rejection.** A first pass keyed the per-tree signature on - `AnnotatedTypeMirror.toString()` and concluded the pre-flow type was "unstable 25–40% (leaves) / - 26–59% (compound, even checking-phase)," i.e. that no tree-keyed cache could be sound. **That was - a measurement artifact, not a property of the code.** `toString()` conflates two non-issues with - real instability: (i) *cross-hierarchy completeness* — a primitive literal printing as `int` vs - `@Initialized int` is the Initialization hierarchy's annotation absent vs present, not a - within-hierarchy disagreement; and (ii) the comparison read `getTopAnnotations()` (a `Set` with - unstable iteration order) *positionally*, so slot *i* compared different hierarchies across calls. - Re-measured *per hierarchy* (keyed by top-annotation name; counting "present-disagree" = same - hierarchy, two different non-∅ annotations, and "completeness" = ∅-vs-present, separately): + **Correction to an earlier rejection (and to a wrong root-cause in a prior revision of this very + entry).** A first pass keyed the per-tree signature on `AnnotatedTypeMirror.toString()` and a + *static* `IdentityHashMap`, and concluded the pre-flow type was "unstable 25–59%," i.e. + that no tree-keyed cache could be sound. **That was a measurement artifact.** Two confounds, both + in the instrumentation, not the code: (i) `toString()` conflates *cross-hierarchy completeness* — + a primitive literal printing as `int` vs `@Initialized int` is the Initialization hierarchy's + annotation absent vs present, not a within-hierarchy disagreement; and (ii) — the big one — the + per-tree map was **`static`, shared across the *multiple sub-factories* a checker runs.** A + `nullness` compile runs four `GenericAnnotatedTypeFactory` instances + (`NullnessNoInitAnnotatedTypeFactory`, `InitializationAnnotatedTypeFactory`, + `InitializationFieldAccessAnnotatedTypeFactory`, `KeyForAnnotatedTypeFactory`), each with its own + qualifier hierarchy; the static map compared *one type system's* snapshot of a tree against + *another's* (e.g. `@UnknownKeyFor` from the KeyFor factory vs `@Initialized` from the + Initialization factory). (`getTopAnnotations()` itself is **not** the problem — it returns a + build-once `final AnnotationMirrorSet` backed by an `ArrayList` over a `TreeMap`, + i.e. already a stable, deterministic order; an earlier draft of this entry wrongly blamed it.) + Re-measured correctly — *per factory* (instance map) and *per hierarchy* (keyed by top-annotation + name; "present-disagree" = same hierarchy, two different non-∅ annotations; "completeness" = + ∅-vs-present): | category | all-systems (120f) | loops | | --- | --- | --- | - | value leaf (literal / var / field / param id) | disagree 0, compl 0 / 69,557 repeats | 0 / 0 / 207,940 | - | compound expression | disagree 211 (0.46%), compl 0 / 46,166 | 0 / 0 / 57,928 | - | type-name identifier (class/enum) | 86 (0.7%) | 0 | - - So per hierarchy the pre-flow type is **stable**: value leaves never disagree, compound - expressions disagree ~0.46% (the genuinely context-dependent residual — `NewClass`/`NewArray`/ - conditionals, the #602 set in #3 below), type-name identifiers ~0.7% (excludable by element - kind). The flow-dependence is entirely in 2b, exactly as the split assumed. **This reopens the - split cache as a *sound* candidate** — provided it returns a deep copy (callers mutate) and skips - the ~0.5–0.7% context-dependent kinds. + | value leaf (literal / var / field / param id) | disagree 0, compl 0 / 58,447 repeats | 0 / 0 / 130,894 | + | compound expression | disagree 213 (0.69%), compl 0 / 30,949 | 0 / 0 / 33,604 | + | type-name identifier (class/enum) | excluded (≈0.7% context-dependent) | — | + + So per factory, per hierarchy, the pre-flow type is **stable**: value leaves never disagree or + under-annotate, compound expressions disagree ~0.69% (the genuinely context-dependent residual — + `NewClass`/`NewArray`/conditionals, the #602 set in #3 below), type-name identifiers ~0.7% + (excludable by element kind). The flow-dependence is entirely in 2b, exactly as the split + assumed. **This reopens the split cache as a *sound* candidate** — provided it is per-factory + (each ATF has its own cache, as all the existing tree caches already are), returns a deep copy + (callers mutate), and skips the ~0.5–0.7% context-dependent kinds. **Value is NOT yet confirmed; do an implemented-cache A/B before adoption.** Instrumented 2a self-time for value leaves alone is ~0.55 s (~4% of a 12.6 s all-systems compile) and ~1.16 s @@ -2107,8 +2117,11 @@ Capture format: hot method, hypothesis, blockers. compound kinds) and measure deterministic allocation + wall on all-systems and loops. Until that A/B exists this is a *candidate*, not a win. (The companion "phase-scoped full cache" idea was rejected on the same flawed `toString()` measure and should likewise be re-measured per hierarchy - if revisited.) **Method lesson: never measure annotated-type stability via `toString()` — compare - per hierarchy, and never index a qualifier `Set` positionally across calls.** + if revisited.) **Method lessons: (1) never measure annotated-type stability via `toString()` — + compare per hierarchy (top-name → annotation), since `toString()` conflates cross-hierarchy + completeness; (2) a checker runs *several* `AnnotatedTypeFactory` instances — never key + instrumentation (or a cache) on `Tree` in `static` state, or you compare/mix different type + systems' results for the same tree.** 3. **Split flow-independent structure from flow-dependent annotations — MEASURED, ALREADY IMPLEMENTED (June 2026).** The hypothesis was: `fromExpression` (~24% inclusive) builds a deterministic-per-tree skeleton, so cache the frozen skeleton and re-apply only the From ba53d25f77518dd61754ca5d29167c520b4e7149 Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 21:44:08 -0400 Subject: [PATCH 06/12] performance-notes: build the pre-flow getAnnotatedType cache, then reject it Resolve the "candidate, pending A/B" status of the pre-flow split cache by actually building it and measuring. A working per-factory cache (frozen pre-flow types, populated in addComputedTypeAnnotations, served by an overridden getAnnotatedType that re-applies only the flow step, toggled by -Dcf.preflowcache) was implemented and tested. Rejected for two independent reasons: - Unsound across checkers. IndexTest fails (PredecrementTest: an extra array.access.unsafe.low). UpperBound/LowerBound/Optional/Lock/Signedness override addComputedTypeAnnotations to add flow-dependent annotations after super; intercepting at getAnnotatedType bypasses that subclass logic on a hit. A nullness-only diagnostic diff was clean, which is why nullness validation missed it. - Flat where sound. Deterministic A/B (nullness): all-systems alloc -1.6% / wall ~0; loops alloc +2.0% (worse) / wall -4.7%. The per-hit deepCopy+freeze overhead cancels the saved 2a work; the projected 4-15% did not appear. Update the skill: stability != cacheable; a getAnnotatedType cache must compose with subclass addComputedTypeAnnotations overrides; validate on the override-checkers (Index/Lock/Optional/Signedness), not just nullness. Co-Authored-By: Claude Opus 4.8 --- .claude/skills/cf-performance/SKILL.md | 8 +++- docs/developer/performance-notes.md | 51 +++++++++++++++++--------- 2 files changed, 41 insertions(+), 18 deletions(-) diff --git a/.claude/skills/cf-performance/SKILL.md b/.claude/skills/cf-performance/SKILL.md index cdaef51b5dbb..4e258387e308 100644 --- a/.claude/skills/cf-performance/SKILL.md +++ b/.claude/skills/cf-performance/SKILL.md @@ -392,7 +392,13 @@ but cost ≈10% wall clock (far more than its hit-rate delta implied). Cut already caches.** - **Two traps when measuring whether a per-tree type is stable enough to cache** (both produced a *wrong, committed* conclusion this session — the pre-flow `getAnnotatedType` split-cache was - rejected as "25–59% unstable," then re-measuring correctly showed value leaves are **0%** unstable): + rejected as "25–59% unstable," then re-measuring correctly showed value leaves are **0%** unstable). + Postscript: even with 0% instability the built cache was *still* rejected — it was unsound for + checkers that override `addComputedTypeAnnotations` (Index/Lock/Optional/Signedness add annotations + after `super`, which a `getAnnotatedType` cache hit bypasses → `IndexTest` failed) and flat-to-mixed + on nullness where it was correct. **Stability ≠ cacheable: a `getAnnotatedType` cache must compose + with subclass overrides, and the per-hit `deepCopy` can cancel the saved work. Validate correctness + on the override-checkers, not just nullness.** The measurement traps: 1. **Compare PER HIERARCHY, never via `AnnotatedTypeMirror.toString()`.** `toString()` over-reports instability by counting *cross-hierarchy completeness* — an `int` literal printing as `int` vs `@Initialized int` is just the Initialization hierarchy's annotation absent vs present, harmless diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index e117ac76dcd9..7c961e097329 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -2066,8 +2066,8 @@ Capture format: hot method, hypothesis, blockers. the soundness crux flagged here. And the instability is *highest where the redundancy is highest* (loops). A per-analysis memo would cache stale/wrong types; a per-*iteration* memo would be sound but the within-iteration redundancy is ~0. Low value and unsound — do not pursue. - 2. **Pre-flow expression-type ("split") cache — RE-MEASURED; a genuine candidate, value pending an - A/B (June 2026).** Split `addComputedTypeAnnotations(Tree, ATM)` at the `if (this.useFlow)` + 2. **Pre-flow expression-type ("split") cache — RE-MEASURED, then BUILT AND REJECTED (June 2026).** + Split `addComputedTypeAnnotations(Tree, ATM)` at the `if (this.useFlow)` boundary into the prefix 2a (`applyQualifierParameterDefaults` → tree/type annotators → `defaults.annotate`) and the flow suffix 2b (`getInferredValueFor` + `applyInferredAnnotations`). Hypothesis: cache the *pre-flow* (post-2a) type per tree and recompute only 2b, since 2b is where @@ -2103,21 +2103,38 @@ Capture format: hot method, hypothesis, blockers. under-annotate, compound expressions disagree ~0.69% (the genuinely context-dependent residual — `NewClass`/`NewArray`/conditionals, the #602 set in #3 below), type-name identifiers ~0.7% (excludable by element kind). The flow-dependence is entirely in 2b, exactly as the split - assumed. **This reopens the split cache as a *sound* candidate** — provided it is per-factory - (each ATF has its own cache, as all the existing tree caches already are), returns a deep copy - (callers mutate), and skips the ~0.5–0.7% context-dependent kinds. - - **Value is NOT yet confirmed; do an implemented-cache A/B before adoption.** Instrumented 2a - self-time for value leaves alone is ~0.55 s (~4% of a 12.6 s all-systems compile) and ~1.16 s - (~15%) on loop-heavy code, vs ~0.05–0.17 s deepCopy — a favorable projected ratio, and compound - 2a (not separately timed) is larger. But projected savings have repeatedly evaporated in A/B this - session (the String→`Name` flat result; #4 applied-defaults). The next step is to *build* the - post-2a cache (store the type just before the `useFlow` block; on hit `deepCopy` + run only 2b; - key by `Tree`; clear per CU like the other tree caches; skip type-name identifiers and the #602 - compound kinds) and measure deterministic allocation + wall on all-systems and loops. Until that - A/B exists this is a *candidate*, not a win. (The companion "phase-scoped full cache" idea was - rejected on the same flawed `toString()` measure and should likewise be re-measured per hierarchy - if revisited.) **Method lessons: (1) never measure annotated-type stability via `toString()` — + assumed. The per-hierarchy stability made it look like a sound candidate. + + **BUILT AND REJECTED (June 2026) — unsound across checkers, and flat where it is sound.** A + working cache was implemented: a per-factory `IdentityHashMap` of + frozen pre-flow types, populated just before the `useFlow` block in + `GenericAnnotatedTypeFactory.addComputedTypeAnnotations`; `getAnnotatedType` was overridden to, + on a hit for a value-leaf, return `cached.deepCopy()` + re-applied flow (2b); cleared per CU; + toggled by `-Dcf.preflowcache`. Two findings killed it: + - **Unsound for any checker that overrides `addComputedTypeAnnotations`.** `IndexTest` fails + (`PredecrementTest.java:8`, an extra `array.access.unsafe.low`). `UpperBoundAnnotatedTypeFactory` + (and Lower Bound, Optional, Lock, Signedness) override `addComputedTypeAnnotations` to add + flow-dependent annotations **after** `super` — e.g. Index pulls the Value Checker's type and + calls `addUpperBoundTypeFromValueType` (its own comment cites `"int i = 1; --i;"`, exactly the + failing test). Intercepting at `getAnnotatedType` and re-applying only the GATF-level flow step + bypasses that subclass logic, so cache hits drop the index refinement. Caching *inside* + `addComputedTypeAnnotations` so the subclass tail still runs would need a deep "replace + annotations onto the passed-in type" and only saves 2a (not `fromExpression`) — more complexity + for a smaller win. (A diff of cache-on vs -off diagnostics on nullness corpora *was* clean — + the bug is specific to the subclass-override checkers, which is why nullness-only validation + missed it.) + - **Flat-to-mixed even where it is sound (nullness).** Deterministic A/B (median of 5 alloc; + 2nd-best of 4 wall), cache off vs on: all-systems alloc 2658.7 → 2616.1 MB (−1.6%), wall + 12.12 → 12.10 s (~0); loops alloc 1078.2 → **1100.1 MB (+2.0%, worse)**, wall 7.18 → 6.84 s + (−4.7%). The projected ~4–15% (raw 2a self-time) did not materialize: the per-hit `deepCopy` + + `freeze` + map overhead roughly cancels the saved 2a, and on the realistic corpus the result + is noise. Do not pursue. **Lesson: per-hierarchy stability proved the cache *could* be sound in + isolation, but a framework-wide `getAnnotatedType` cache must compose with subclass + `addComputedTypeAnnotations` overrides — and even ignoring that, the deepCopy cost makes it a + wash. Validate correctness across the *override* checkers (Index/Lock/Optional/Signedness), not + just nullness.** (The companion "phase-scoped full cache" idea was rejected on the same flawed + `toString()` measure and would hit the same compose-with-overrides wall.) **Method lessons: + (1) never measure annotated-type stability via `toString()` — compare per hierarchy (top-name → annotation), since `toString()` conflates cross-hierarchy completeness; (2) a checker runs *several* `AnnotatedTypeFactory` instances — never key instrumentation (or a cache) on `Tree` in `static` state, or you compare/mix different type From e25690d5782b4095bfe6d22e60eb0bac2eaff138 Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 22:19:15 -0400 Subject: [PATCH 07/12] performance-notes: mark getAnnotatedType #2 closed in the Short list index Move the pre-flow split cache from "Open, low-value" to "Closed, do not re-propose" now that it has been built and rejected. Co-Authored-By: Claude Opus 4.8 --- docs/developer/performance-notes.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index 7c961e097329..1c58248bd714 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -1948,12 +1948,12 @@ super-linears — pathological depth, shallow in real code), or research-scale Index (entries are interleaved below; each is tagged with its status inline): - **Open, low-value:** `qualifiedNameCache` backing map; typeinference8 resolution #3 (`getInstantiatedVariables`) and #4 (`getSmallestDependencySet`); the `cond` post-dataflow - conditional cache and `inherit` asSuper depth (size-sweep); `getAnnotatedType` #2 (scope-bounded - cache, same soundness crux) and #6 (parallelism). + conditional cache and `inherit` asSuper depth (size-sweep); `getAnnotatedType` #6 (parallelism). - **Open but correctness/cost-blocked:** `CFAbstractStore` content hash; lazy JDK-stub cascade; the immutability allocation win (load-bearing copy — see narrative). - **Closed, do not re-propose:** PR #1829 incorporation worklist (shipped); `getAnnotatedType` #1 - (per-analysis gvff memo), #3 (already implemented), #4 (applied-defaults), #5 + (per-analysis gvff memo), #2 (pre-flow split cache — built & rejected: unsound across + override-checkers + flat), #3 (already implemented), #4 (applied-defaults), #5 (`methodFromUse`/`constructorFromUse`); typeinference8 resolution #1 (dependency graph) and #2 (`saveBounds`); `AbstractAnalysis.getValue` subnode test (see Tried and rejected). From 76f24e9830449194d1be4fbf5c560766c9252246 Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 22:35:42 -0400 Subject: [PATCH 08/12] performance-notes: record first Guava hotspot JFR (inference is a top-2 cost) Profiled the Nullness Checker on the Guava `guava` module (625 files, 7,814 JFR samples) -- the first hotspot trace of a large heavy-generics codebase (Guava is the 994-work-unit case used only for correctness/budget so far). Leaf self-time generalizes: same flat profile as checkNullness, no CF leaf above 3.85% (IdentityHashMap lookups, ATM traversal, apiComplete, defaulting, AnnotatedTypeCopier, createType). New fact: type-argument inference is ~23% inclusive (methodFromUse 24.6% -> inferTypeArgs 23.0% -> reduceOneStep 14.1%), where it is negligible on all-systems/checkNullness -- but 50% of that is getAnnotatedType and 28% is the already-shipped #1829 incorporation, so it reinforces the Short list rather than opening a new lever. Note Guava (or jspecify-conformance) as the venue for any future inference work. Co-Authored-By: Claude Opus 4.8 --- docs/developer/performance-notes.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index 1c58248bd714..9922eaba7e76 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -2287,6 +2287,32 @@ wins did not move single-compile time — their value is GC pressure at scale); (2) The largest non-obvious CPU slice is CF driving javac (symbol completion + name decoding + tree/path walks), bigger than dataflow + stubs + visitor combined. +**Guava cross-check (first hotspot JFR of a large heavy-generics codebase, June 2026).** +`test-guava-nullness.sh` (Nullness Checker on the `guava` module, 625 files; JFR injected +via the forked-compiler `-J` args in the `checkerframework-local` profile; 7,814 +ExecutionSamples). Two findings: +- **The leaf self-time profile generalizes — nothing new at the leaf.** Same flat shape as + `checkNullness`: no CF leaf above **3.85%**; the top is `IdentityHashMap.get`/`put` (≈6.8% + combined), the ATM traversal (`AnnotatedTypeScanner.scan`/`visitDeclared`/`reduce` ≈7%), + `Symbol.apiComplete`, `DefaultApplierElementImpl.scan`, `AnnotatedTypeCopier.visitDeclared`, + `AnnotatedTypeMirror.createType`. Allocation: `[Ljava.lang.Object;` 32%, `ArrayList` 8.7%, + `IdentityHashMap` 5.5%, `AnnotationMirrorSet` 4.5% — the same ATM-pipeline allocation. +- **New *fact* (not a new lever): type-argument inference is a top-2 inclusive cost on heavy + generics, where it is negligible on `all-systems`/`checkNullness`.** `getAnnotatedType` 50% + inclusive (as usual), but then `methodFromUse` **24.6%** → `inferTypeArgs` **23.0%** → + `InvocationTypeInference.infer` 22.4% → `ConstraintSet.reduceOneStep` 14.1%. Decomposing the + 23% inference slice (`cooccur`): **50% of it is under `getAnnotatedType`** (building + argument/receiver/bound types) and **28% under `incorporateToFixedPoint`** (the machinery + PR #1829 already optimized); the inference-*specific* self-time (`TypeConstraint.hashCode`, + `ConstraintSet`, constraint-collection churn — `LinkedHashMap`/`ListBuffer`/`List$2` ≈6% of + allocation) is small and diffuse. So Guava **reinforces** rather than overturns the Short + list: the biggest lever is still `getAnnotatedType` (now shown to dominate inference too, not + just checking), inference's big win already shipped (#1829), and the resolution items (#3/#4) + stay low-value — their self-time is tiny even here. Takeaway for future sessions: `all-systems` + and `checkNullness` **under-represent inference**; profile **Guava (or jspecify-conformance)** + for any inference work, but expect the cost to be the type pipeline it drives, not a clean + inference-specific frame. + Open venues, roughly by tractability: - **Reduce ATM deep copying (`AnnotatedTypeCopier.visit` = 22% of `Object[]`).** From fb5739a665696144ed52c7f491a9fa95f428cd2b Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Mon, 22 Jun 2026 22:57:26 -0400 Subject: [PATCH 09/12] performance-notes/skill: consolidate getAnnotatedType cache learnings Editorial pass for review: rewrite the pre-flow split-cache entry and the companion skill lesson to present the final understanding coherently (built & rejected; stability != cacheable; the two measurement traps; getTopAnnotations is already stable), dropping the incremental "prior revision of this entry" framing. No new findings; content-equivalent. Co-Authored-By: Claude Opus 4.8 --- .claude/skills/cf-performance/SKILL.md | 18 ++--- docs/developer/performance-notes.md | 107 ++++++++++--------------- 2 files changed, 53 insertions(+), 72 deletions(-) diff --git a/.claude/skills/cf-performance/SKILL.md b/.claude/skills/cf-performance/SKILL.md index 4e258387e308..c311a49c31cc 100644 --- a/.claude/skills/cf-performance/SKILL.md +++ b/.claude/skills/cf-performance/SKILL.md @@ -390,15 +390,15 @@ but cost ≈10% wall clock (far more than its hit-rate delta implied). Cut (`CheckerFrameworkAnnotationMirror`) already caches the decoded interned name, so the decode the change "removed" was never happening. **Before optimizing a lookup, ask what the common carrier already caches.** -- **Two traps when measuring whether a per-tree type is stable enough to cache** (both produced a - *wrong, committed* conclusion this session — the pre-flow `getAnnotatedType` split-cache was - rejected as "25–59% unstable," then re-measuring correctly showed value leaves are **0%** unstable). - Postscript: even with 0% instability the built cache was *still* rejected — it was unsound for - checkers that override `addComputedTypeAnnotations` (Index/Lock/Optional/Signedness add annotations - after `super`, which a `getAnnotatedType` cache hit bypasses → `IndexTest` failed) and flat-to-mixed - on nullness where it was correct. **Stability ≠ cacheable: a `getAnnotatedType` cache must compose - with subclass overrides, and the per-hit `deepCopy` can cancel the saved work. Validate correctness - on the override-checkers, not just nullness.** The measurement traps: +- **Caching a per-tree annotated type: stability ≠ cacheable, and two measurement traps.** The + pre-flow `getAnnotatedType` split-cache (June 2026, *Tried and rejected* → getAnnotatedType #2) is + the cautionary tale. **Stability ≠ cacheable:** even after correctly measuring value-leaf pre-flow + types as **0%** unstable, the built cache was rejected — it was unsound for checkers that override + `addComputedTypeAnnotations` (Index/Lock/Optional/Signedness add annotations after `super`, which a + `getAnnotatedType` cache hit bypasses → `IndexTest` failed), and flat-to-mixed on nullness where it + was correct. A framework-level type cache must **compose with subclass overrides**, and the per-hit + `deepCopy` can cancel the saved work — so validate correctness on the override-checkers, not just + nullness. Two traps nearly produced the *opposite* wrong verdict ("25–59% unstable, unsound") first: 1. **Compare PER HIERARCHY, never via `AnnotatedTypeMirror.toString()`.** `toString()` over-reports instability by counting *cross-hierarchy completeness* — an `int` literal printing as `int` vs `@Initialized int` is just the Initialization hierarchy's annotation absent vs present, harmless diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index 9922eaba7e76..2c705b489f08 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -2066,32 +2066,12 @@ Capture format: hot method, hypothesis, blockers. the soundness crux flagged here. And the instability is *highest where the redundancy is highest* (loops). A per-analysis memo would cache stale/wrong types; a per-*iteration* memo would be sound but the within-iteration redundancy is ~0. Low value and unsound — do not pursue. - 2. **Pre-flow expression-type ("split") cache — RE-MEASURED, then BUILT AND REJECTED (June 2026).** - Split `addComputedTypeAnnotations(Tree, ATM)` at the `if (this.useFlow)` - boundary into the prefix 2a (`applyQualifierParameterDefaults` → tree/type annotators → - `defaults.annotate`) and the flow suffix 2b (`getInferredValueFor` + `applyInferredAnnotations`). - Hypothesis: cache the *pre-flow* (post-2a) type per tree and recompute only 2b, since 2b is where - flow lives. - - **Correction to an earlier rejection (and to a wrong root-cause in a prior revision of this very - entry).** A first pass keyed the per-tree signature on `AnnotatedTypeMirror.toString()` and a - *static* `IdentityHashMap`, and concluded the pre-flow type was "unstable 25–59%," i.e. - that no tree-keyed cache could be sound. **That was a measurement artifact.** Two confounds, both - in the instrumentation, not the code: (i) `toString()` conflates *cross-hierarchy completeness* — - a primitive literal printing as `int` vs `@Initialized int` is the Initialization hierarchy's - annotation absent vs present, not a within-hierarchy disagreement; and (ii) — the big one — the - per-tree map was **`static`, shared across the *multiple sub-factories* a checker runs.** A - `nullness` compile runs four `GenericAnnotatedTypeFactory` instances - (`NullnessNoInitAnnotatedTypeFactory`, `InitializationAnnotatedTypeFactory`, - `InitializationFieldAccessAnnotatedTypeFactory`, `KeyForAnnotatedTypeFactory`), each with its own - qualifier hierarchy; the static map compared *one type system's* snapshot of a tree against - *another's* (e.g. `@UnknownKeyFor` from the KeyFor factory vs `@Initialized` from the - Initialization factory). (`getTopAnnotations()` itself is **not** the problem — it returns a - build-once `final AnnotationMirrorSet` backed by an `ArrayList` over a `TreeMap`, - i.e. already a stable, deterministic order; an earlier draft of this entry wrongly blamed it.) - Re-measured correctly — *per factory* (instance map) and *per hierarchy* (keyed by top-annotation - name; "present-disagree" = same hierarchy, two different non-∅ annotations; "completeness" = - ∅-vs-present): + 2. **Pre-flow expression-type ("split") cache — BUILT AND REJECTED (June 2026).** Hypothesis: + `addComputedTypeAnnotations(Tree, ATM)` splits at the `if (this.useFlow)` boundary into a prefix + 2a (`applyQualifierParameterDefaults` → tree/type annotators → `defaults.annotate`) and a flow + suffix 2b (`getInferredValueFor` + `applyInferredAnnotations`); since flow lives only in 2b, cache + the *pre-flow* (post-2a) type per tree and recompute only 2b. Per-factory, per-hierarchy + measurement shows the pre-flow type *is* stable, so the cache looked sound: | category | all-systems (120f) | loops | | --- | --- | --- | @@ -2099,46 +2079,47 @@ Capture format: hot method, hypothesis, blockers. | compound expression | disagree 213 (0.69%), compl 0 / 30,949 | 0 / 0 / 33,604 | | type-name identifier (class/enum) | excluded (≈0.7% context-dependent) | — | - So per factory, per hierarchy, the pre-flow type is **stable**: value leaves never disagree or - under-annotate, compound expressions disagree ~0.69% (the genuinely context-dependent residual — - `NewClass`/`NewArray`/conditionals, the #602 set in #3 below), type-name identifiers ~0.7% - (excludable by element kind). The flow-dependence is entirely in 2b, exactly as the split - assumed. The per-hierarchy stability made it look like a sound candidate. - - **BUILT AND REJECTED (June 2026) — unsound across checkers, and flat where it is sound.** A - working cache was implemented: a per-factory `IdentityHashMap` of - frozen pre-flow types, populated just before the `useFlow` block in - `GenericAnnotatedTypeFactory.addComputedTypeAnnotations`; `getAnnotatedType` was overridden to, - on a hit for a value-leaf, return `cached.deepCopy()` + re-applied flow (2b); cleared per CU; - toggled by `-Dcf.preflowcache`. Two findings killed it: + (Value leaves never disagree or under-annotate; the compound ~0.69% residual is the genuinely + context-dependent `NewClass`/`NewArray`/conditional set — the #602 trees of #3 below; type-name + identifiers ~0.7%, excludable by element kind.) A working cache was then implemented — a + per-factory `IdentityHashMap` of frozen pre-flow types populated just + before the `useFlow` block, with `getAnnotatedType` overridden to return `cached.deepCopy()` + + re-applied 2b on a value-leaf hit, cleared per CU, toggled by `-Dcf.preflowcache`. **Two + independent findings killed it:** - **Unsound for any checker that overrides `addComputedTypeAnnotations`.** `IndexTest` fails - (`PredecrementTest.java:8`, an extra `array.access.unsafe.low`). `UpperBoundAnnotatedTypeFactory` + (`PredecrementTest.java:8`, a spurious `array.access.unsafe.low`). `UpperBoundAnnotatedTypeFactory` (and Lower Bound, Optional, Lock, Signedness) override `addComputedTypeAnnotations` to add - flow-dependent annotations **after** `super` — e.g. Index pulls the Value Checker's type and - calls `addUpperBoundTypeFromValueType` (its own comment cites `"int i = 1; --i;"`, exactly the - failing test). Intercepting at `getAnnotatedType` and re-applying only the GATF-level flow step - bypasses that subclass logic, so cache hits drop the index refinement. Caching *inside* + flow-dependent annotations **after** `super` — Index pulls the Value Checker's type and calls + `addUpperBoundTypeFromValueType` (its comment cites `"int i = 1; --i;"`, exactly the failing + test). Intercepting at `getAnnotatedType` and re-applying only the GATF-level flow step bypasses + that subclass tail, so hits drop the index refinement. Caching *inside* `addComputedTypeAnnotations` so the subclass tail still runs would need a deep "replace - annotations onto the passed-in type" and only saves 2a (not `fromExpression`) — more complexity - for a smaller win. (A diff of cache-on vs -off diagnostics on nullness corpora *was* clean — - the bug is specific to the subclass-override checkers, which is why nullness-only validation - missed it.) - - **Flat-to-mixed even where it is sound (nullness).** Deterministic A/B (median of 5 alloc; - 2nd-best of 4 wall), cache off vs on: all-systems alloc 2658.7 → 2616.1 MB (−1.6%), wall - 12.12 → 12.10 s (~0); loops alloc 1078.2 → **1100.1 MB (+2.0%, worse)**, wall 7.18 → 6.84 s - (−4.7%). The projected ~4–15% (raw 2a self-time) did not materialize: the per-hit `deepCopy` + - `freeze` + map overhead roughly cancels the saved 2a, and on the realistic corpus the result - is noise. Do not pursue. **Lesson: per-hierarchy stability proved the cache *could* be sound in - isolation, but a framework-wide `getAnnotatedType` cache must compose with subclass - `addComputedTypeAnnotations` overrides — and even ignoring that, the deepCopy cost makes it a - wash. Validate correctness across the *override* checkers (Index/Lock/Optional/Signedness), not - just nullness.** (The companion "phase-scoped full cache" idea was rejected on the same flawed - `toString()` measure and would hit the same compose-with-overrides wall.) **Method lessons: - (1) never measure annotated-type stability via `toString()` — - compare per hierarchy (top-name → annotation), since `toString()` conflates cross-hierarchy - completeness; (2) a checker runs *several* `AnnotatedTypeFactory` instances — never key - instrumentation (or a cache) on `Tree` in `static` state, or you compare/mix different type - systems' results for the same tree.** + annotations onto the passed-in type" and saves only 2a (not `fromExpression`) — more complexity, + smaller win. A nullness-only diagnostic diff (cache on vs off) was clean, which is exactly why + nullness-only validation missed it. + - **Flat-to-mixed even where it is sound (nullness).** Deterministic A/B (median-of-5 alloc; + 2nd-best-of-4 wall), off vs on: all-systems alloc 2658.7 → 2616.1 MB (−1.6%), wall 12.12 → + 12.10 s (~0); loops alloc 1078.2 → **1100.1 MB (+2.0%, worse)**, wall 7.18 → 6.84 s (−4.7%). The + projected ~4–15% (raw 2a self-time) did not materialize — the per-hit `deepCopy` + `freeze` + + map overhead roughly cancels the saved 2a, and the realistic corpus is noise. + + **Do not pursue.** The companion "phase-scoped full cache" idea would hit the same + compose-with-overrides wall. **Lessons:** + - *Stability ≠ cacheable.* Per-hierarchy stability proved the cache could be sound in isolation, + but a framework-level `getAnnotatedType` cache must **compose with subclass + `addComputedTypeAnnotations` overrides**, and even then the `deepCopy` cost makes it a wash. + Validate such a change on the override-checkers (Index/Lock/Optional/Signedness), not just + nullness. + - *Measuring per-tree type stability has two traps* (both nearly produced a false "unstable + 25–59%, unsound" verdict): **(1)** never compare via `AnnotatedTypeMirror.toString()` — it + conflates *cross-hierarchy completeness* (a literal printing as `int` vs `@Initialized int` is + the Initialization annotation absent-vs-present, not a within-hierarchy change) with real + disagreement; compare per hierarchy as {top-name → annotation}. **(2)** a checker runs *several* + `AnnotatedTypeFactory` instances (a `nullness` compile drives `NullnessNoInit`, `Initialization`, + `InitializationFieldAccess`, `KeyFor`) — never key instrumentation or a cache on `Tree` in + `static` state, or you compare one type system's snapshot against another's. (Note: + `getTopAnnotations()` is *not* an ordering hazard — it returns a build-once `ArrayList`-backed + `AnnotationMirrorSet` over a `TreeMap`, a stable deterministic order.) 3. **Split flow-independent structure from flow-dependent annotations — MEASURED, ALREADY IMPLEMENTED (June 2026).** The hypothesis was: `fromExpression` (~24% inclusive) builds a deterministic-per-tree skeleton, so cache the frozen skeleton and re-apply only the From 25b08584876c9f51803b0ed3737f1a34da4480c0 Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Tue, 23 Jun 2026 00:02:10 -0400 Subject: [PATCH 10/12] WIP: copy-on-write ATMs to make cache boundary flips sound (prototype) Prototype of copy-on-write that unblocks the immutability allocation win (return shared frozen cache masters instead of deepCopy()), under -Dcf.cow. Design: the 6 post-pipeline caches return cowCopy() (a non-frozen shallow copy sharing the master's frozen children) instead of deepCopy(); the ~13 child accessors and the three fixupBoundAnnotations lazily unshare a frozen child of a non-frozen parent (cowChild/cowChildren); a per-node cowDirty flag keeps the COW scan off the hot path for non-cache types. Validated: ElementTypeCacheWildcardBound, all-systems 269 (identical diagnostics), full Guava nullness build (BUILD SUCCESS, 0 crashes). Measured (all-systems 269): allocation -4.8%, wall +5.3%. Correctness-complete but wall-negative; off by default. Co-Authored-By: Claude Opus 4.8 --- .../framework/type/AnnotatedTypeFactory.java | 15 +-- .../framework/type/AnnotatedTypeMirror.java | 93 +++++++++++++++++++ 2 files changed, 102 insertions(+), 6 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java index 350c59635874..89f0a2bd2cdc 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java @@ -1579,7 +1579,7 @@ public AnnotatedTypeMirror getAnnotatedType(Element elt) { if (useCache) { AnnotatedTypeMirror cached = elementTypeCache.get(elt); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } // Annotations explicitly written in the source code, @@ -1831,7 +1831,7 @@ public AnnotatedTypeMirror fromElement(Element elt) { if (shouldCache) { AnnotatedTypeMirror cached = elementCache.get(elt); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } if (elt.getKind() == ElementKind.PACKAGE) { @@ -1950,7 +1950,7 @@ private AnnotatedTypeMirror fromMember(Tree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromMemberTreeCache.get(tree); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } AnnotatedTypeMirror result = TypeFromTree.fromMember(this, tree); @@ -2046,7 +2046,7 @@ private AnnotatedTypeMirror fromExpression(ExpressionTree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromExpressionTreeCache.get(tree); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } @@ -2079,7 +2079,7 @@ private AnnotatedTypeMirror fromExpression(ExpressionTree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromTypeTreeCache.get(tree); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } @@ -2850,7 +2850,10 @@ && shouldCacheMethodAsMemberOf() AnnotatedExecutableType cachedMethodType = cacheKey == null ? null : methodAsMemberOfCache.get(cacheKey); if (cachedMethodType != null) { - methodType = cachedMethodType.deepCopy(); + methodType = + AnnotatedTypeMirror.COW + ? (AnnotatedExecutableType) cachedMethodType.cowCopy() + : cachedMethodType.deepCopy(); } else { methodType = computeMethodTypeAsMemberOf(tree, methodElt, receiverType, inferTypeArgs); if (cacheKey != null) { diff --git a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java index aa56faa2d810..654230bba71b 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java @@ -910,6 +910,79 @@ protected final void checkMutable() { } } + /** + * Experimental copy-on-write toggle ({@code -Dcf.cow}). When on, post-pipeline caches hand out + * a {@link #shallowCopy()} (a fresh, non-frozen root that shares the master's frozen children) + * instead of a {@link #deepCopy()}, and the child accessors below lazily unshare a frozen child + * of a non-frozen parent ({@link #cowChild}/{@link #cowChildren}). Mutation therefore copies + * only the spine it touches; read-only hits copy one node. Off by default (behavior unchanged). + */ + static final boolean COW = Boolean.getBoolean("cf.cow"); + + /** + * COW fast-path flag: true only for a type that may transitively hold a frozen (shared) child — + * i.e. a {@link #cowCopy()} of a frozen cache master, or a child unshared from one. A type + * without this flag was built fresh and has no frozen children, so the COW accessors can skip + * the per-call scan entirely. This keeps {@link #cowChild}/{@link #cowChildren} off the hot + * path for the overwhelming majority of (non-cache) types. + */ + private boolean cowDirty = false; + + /** + * Returns a non-frozen shallow copy of this (frozen) type, marked {@link #cowDirty} because it + * shares this type's frozen children. Used at cache boundaries and by the COW accessors. + * + * @return a cow-dirty shallow copy of this type + */ + final AnnotatedTypeMirror cowCopy() { + AnnotatedTypeMirror c = shallowCopy(); + c.cowDirty = true; + return c; + } + + /** + * Copy-on-write a single child component. If COW is on and {@code this} is non-frozen but + * {@code child} is frozen (shared from a cache master), returns a fresh non-frozen shallow copy + * that the caller stores back into its field; otherwise returns {@code child} unchanged. + * + * @param the static type of the child + * @param child a child component of this type, or null + * @return an unshared shallow copy when COW must unshare, else {@code child} + */ + @SuppressWarnings("unchecked") + final @Nullable T cowChild(@Nullable T child) { + if (COW && cowDirty && !this.frozen && child != null && child.isFrozen()) { + return (T) child.cowCopy(); + } + return child; + } + + /** + * Copy-on-write a list of child components. Returns a new list with any frozen elements + * replaced by unshared shallow copies (when COW is on and {@code this} is non-frozen); + * otherwise returns {@code children} unchanged. + * + * @param children a list of child components + * @return a list with frozen elements unshared, or {@code children} unchanged + */ + @SuppressWarnings("unchecked") + final List cowChildren(List children) { + if (!COW || !cowDirty || this.frozen || children.isEmpty()) { + return children; + } + List result = null; + for (int i = 0; i < children.size(); i++) { + T c = children.get(i); + if (c != null && c.isFrozen()) { + if (result == null) { + result = new ArrayList<>(children); + } + result.set(i, (T) c.cowCopy()); + } + } + return result == null ? children : result; + } + /** * Returns true if this type has been {@linkplain #freeze() frozen}. * @@ -1298,6 +1371,7 @@ public void setTypeArguments(List ts) { */ public List getTypeArguments() { if (typeArgs != null) { + typeArgs = cowChildren(typeArgs); return typeArgs; } @@ -1459,6 +1533,7 @@ public void setEnclosingType(@Nullable AnnotatedDeclaredType enclosingType) { * @return enclosingType the enclosing type, or null if this is a top-level type */ public @Nullable AnnotatedDeclaredType getEnclosingType() { + enclosingType = cowChild(enclosingType); return enclosingType; } @@ -1652,6 +1727,7 @@ public List getParameterTypes() { freezeLazyComponents(paramTypes); } // No need to copy or wrap; it is an unmodifiable list. + paramTypes = cowChildren(paramTypes); return paramTypes; } @@ -1761,6 +1837,7 @@ public AnnotatedTypeMirror getReturnType() { returnTypeComputed = true; freezeLazyComponent(returnType); } + returnType = cowChild(returnType); return returnType; } @@ -1811,6 +1888,7 @@ public AnnotatedTypeMirror getReturnType() { receiverTypeComputed = true; freezeLazyComponent(receiverType); } + receiverType = cowChild(receiverType); return receiverType; } @@ -1858,6 +1936,7 @@ public List getThrownTypes() { freezeLazyComponents(thrownTypes); } // No need to copy or wrap; it is an unmodifiable list. + thrownTypes = cowChildren(thrownTypes); return thrownTypes; } @@ -1907,6 +1986,7 @@ public List getTypeVariables() { freezeLazyComponents(typeVarTypes); } // No need to copy or wrap; it is an unmodifiable list. + typeVarTypes = cowChildren(typeVarTypes); return typeVarTypes; } @@ -2077,6 +2157,7 @@ public AnnotatedTypeMirror getComponentType() { false)); freezeLazyComponent(componentType); } + componentType = cowChild(componentType); return componentType; } @@ -2272,6 +2353,7 @@ public AnnotatedTypeMirror getLowerBound() { freezeLazyComponent(lowerBound); freezeLazyComponent(upperBound); } + lowerBound = cowChild(lowerBound); return lowerBound; } @@ -2291,6 +2373,7 @@ public AnnotatedTypeMirror getLowerBound() { private void fixupBoundAnnotations() { if (!this.getAnnotationsField().isEmpty()) { AnnotationMirrorSet newAnnos = this.getAnnotationsField(); + upperBound = cowChild(upperBound); if (upperBound != null) { upperBound.replaceAnnotations(newAnnos); } @@ -2301,6 +2384,7 @@ private void fixupBoundAnnotations() { // propagate the primary annotation to the type variable because primary annotations // overwrite the upper and lower bounds of type variables when // getUpperBound/getLowerBound is called. + lowerBound = cowChild(lowerBound); if (lowerBound != null) { lowerBound.replaceAnnotations(newAnnos); } @@ -2345,6 +2429,7 @@ public AnnotatedTypeMirror getUpperBound() { freezeLazyComponent(upperBound); freezeLazyComponent(lowerBound); } + upperBound = cowChild(upperBound); return upperBound; } @@ -2660,6 +2745,7 @@ public AnnotatedTypeMirror getSuperBound() { freezeLazyComponent(superBound); freezeLazyComponent(extendsBound); } + this.superBound = cowChild(this.superBound); return this.superBound; } @@ -2694,6 +2780,7 @@ public AnnotatedTypeMirror getExtendsBound() { freezeLazyComponent(extendsBound); freezeLazyComponent(superBound); } + this.extendsBound = cowChild(this.extendsBound); return this.extendsBound; } @@ -2714,9 +2801,11 @@ void freezeComponents() { */ private void fixupBoundAnnotations() { if (!this.getAnnotationsField().isEmpty()) { + superBound = cowChild(superBound); if (superBound != null) { superBound.replaceAnnotations(this.getAnnotationsField()); } + extendsBound = cowChild(extendsBound); if (extendsBound != null) { extendsBound.replaceAnnotations(this.getAnnotationsField()); } @@ -2864,6 +2953,7 @@ public void addAnnotation(AnnotationMirror annotation) { public boolean removeAnnotation(AnnotationMirror a) { boolean ret = super.removeAnnotation(a); if (bounds != null) { + bounds = cowChildren(bounds); for (AnnotatedTypeMirror bound : bounds) { ret |= bound.removeAnnotation(a); } @@ -2879,6 +2969,7 @@ private void fixupBoundAnnotations() { if (!this.getAnnotationsField().isEmpty()) { AnnotationMirrorSet newAnnos = this.getAnnotationsField(); if (bounds != null) { + bounds = cowChildren(bounds); for (AnnotatedTypeMirror bound : bounds) { bound.replaceAnnotations(newAnnos); } @@ -2954,6 +3045,7 @@ public List getBounds() { fixupBoundAnnotations(); freezeLazyComponents(bounds); } + bounds = cowChildren(bounds); return bounds; } @@ -3067,6 +3159,7 @@ public List getAlternatives() { alternatives = Collections.unmodifiableList(res); freezeLazyComponents(alternatives); } + alternatives = cowChildren(alternatives); return alternatives; } From f77bfb52d4c4204960ec92e4456d0d310fac0410 Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Tue, 23 Jun 2026 07:48:29 -0400 Subject: [PATCH 11/12] =?UTF-8?q?WIP:=20wall-clock=20investigation=20of=20?= =?UTF-8?q?COW=20=E2=80=94=20it=20is=20a=20memory=20win,=20not=20a=20wall?= =?UTF-8?q?=20win?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two wall optimizations attempted on the COW prototype: 1. Gate every child accessor with cowActive() (= COW && cowDirty) so non-cache types skip the cowChild call and field write. Removed the field-write cost but did not move wall (the cost was not the write). 2. Flip only elementTypeCache (read-only-majority consumers) instead of all six caches, reverting the full-walk caches (element/fromMember/fromExpression/ fromType/methodAsMemberOf) to deepCopy. Results (all-systems 269): all-six COW = alloc -4.8% / wall +5.3%; elementTypeCache-only = alloc -0.2% / wall +1.8% (noise). No configuration is wall-positive. Structural reasons: the copier is already cheap (~1-2%), so eliminating it cannot beat the global per-accessor cowActive() tax; and the cache consumers fully walk the result (defaulting/annotators), so piecemeal per-child cowCopy is slower than one batched deepCopy and the read-only-skip benefit never materializes. COW is a memory/GC-pressure optimization; for wall clock, the existing deepCopy is already optimal. Co-Authored-By: Claude Opus 4.8 --- .../framework/type/AnnotatedTypeFactory.java | 13 +++--- .../framework/type/AnnotatedTypeMirror.java | 45 ++++++++++--------- 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java index 89f0a2bd2cdc..a5f41e787d6d 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java @@ -1831,7 +1831,7 @@ public AnnotatedTypeMirror fromElement(Element elt) { if (shouldCache) { AnnotatedTypeMirror cached = elementCache.get(elt); if (cached != null) { - return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); + return cached.deepCopy(); } } if (elt.getKind() == ElementKind.PACKAGE) { @@ -1950,7 +1950,7 @@ private AnnotatedTypeMirror fromMember(Tree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromMemberTreeCache.get(tree); if (cached != null) { - return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); + return cached.deepCopy(); } } AnnotatedTypeMirror result = TypeFromTree.fromMember(this, tree); @@ -2046,7 +2046,7 @@ private AnnotatedTypeMirror fromExpression(ExpressionTree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromExpressionTreeCache.get(tree); if (cached != null) { - return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); + return cached.deepCopy(); } } @@ -2079,7 +2079,7 @@ private AnnotatedTypeMirror fromExpression(ExpressionTree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromTypeTreeCache.get(tree); if (cached != null) { - return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); + return cached.deepCopy(); } } @@ -2850,10 +2850,7 @@ && shouldCacheMethodAsMemberOf() AnnotatedExecutableType cachedMethodType = cacheKey == null ? null : methodAsMemberOfCache.get(cacheKey); if (cachedMethodType != null) { - methodType = - AnnotatedTypeMirror.COW - ? (AnnotatedExecutableType) cachedMethodType.cowCopy() - : cachedMethodType.deepCopy(); + methodType = cachedMethodType.deepCopy(); } else { methodType = computeMethodTypeAsMemberOf(tree, methodElt, receiverType, inferTypeArgs); if (cacheKey != null) { diff --git a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java index 654230bba71b..dbf45cb892ad 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java @@ -940,6 +940,11 @@ final AnnotatedTypeMirror cowCopy() { return c; } + /** Whether COW unsharing might be needed for this type's children (cheap hot-path gate). */ + final boolean cowActive() { + return COW && cowDirty; + } + /** * Copy-on-write a single child component. If COW is on and {@code this} is non-frozen but * {@code child} is frozen (shared from a cache master), returns a fresh non-frozen shallow copy @@ -1371,7 +1376,7 @@ public void setTypeArguments(List ts) { */ public List getTypeArguments() { if (typeArgs != null) { - typeArgs = cowChildren(typeArgs); + if (cowActive()) typeArgs = cowChildren(typeArgs); return typeArgs; } @@ -1533,7 +1538,7 @@ public void setEnclosingType(@Nullable AnnotatedDeclaredType enclosingType) { * @return enclosingType the enclosing type, or null if this is a top-level type */ public @Nullable AnnotatedDeclaredType getEnclosingType() { - enclosingType = cowChild(enclosingType); + if (cowActive()) enclosingType = cowChild(enclosingType); return enclosingType; } @@ -1727,7 +1732,7 @@ public List getParameterTypes() { freezeLazyComponents(paramTypes); } // No need to copy or wrap; it is an unmodifiable list. - paramTypes = cowChildren(paramTypes); + if (cowActive()) paramTypes = cowChildren(paramTypes); return paramTypes; } @@ -1837,7 +1842,7 @@ public AnnotatedTypeMirror getReturnType() { returnTypeComputed = true; freezeLazyComponent(returnType); } - returnType = cowChild(returnType); + if (cowActive()) returnType = cowChild(returnType); return returnType; } @@ -1888,7 +1893,7 @@ public AnnotatedTypeMirror getReturnType() { receiverTypeComputed = true; freezeLazyComponent(receiverType); } - receiverType = cowChild(receiverType); + if (cowActive()) receiverType = cowChild(receiverType); return receiverType; } @@ -1936,7 +1941,7 @@ public List getThrownTypes() { freezeLazyComponents(thrownTypes); } // No need to copy or wrap; it is an unmodifiable list. - thrownTypes = cowChildren(thrownTypes); + if (cowActive()) thrownTypes = cowChildren(thrownTypes); return thrownTypes; } @@ -1986,7 +1991,7 @@ public List getTypeVariables() { freezeLazyComponents(typeVarTypes); } // No need to copy or wrap; it is an unmodifiable list. - typeVarTypes = cowChildren(typeVarTypes); + if (cowActive()) typeVarTypes = cowChildren(typeVarTypes); return typeVarTypes; } @@ -2157,7 +2162,7 @@ public AnnotatedTypeMirror getComponentType() { false)); freezeLazyComponent(componentType); } - componentType = cowChild(componentType); + if (cowActive()) componentType = cowChild(componentType); return componentType; } @@ -2353,7 +2358,7 @@ public AnnotatedTypeMirror getLowerBound() { freezeLazyComponent(lowerBound); freezeLazyComponent(upperBound); } - lowerBound = cowChild(lowerBound); + if (cowActive()) lowerBound = cowChild(lowerBound); return lowerBound; } @@ -2373,7 +2378,7 @@ public AnnotatedTypeMirror getLowerBound() { private void fixupBoundAnnotations() { if (!this.getAnnotationsField().isEmpty()) { AnnotationMirrorSet newAnnos = this.getAnnotationsField(); - upperBound = cowChild(upperBound); + if (cowActive()) upperBound = cowChild(upperBound); if (upperBound != null) { upperBound.replaceAnnotations(newAnnos); } @@ -2384,7 +2389,7 @@ private void fixupBoundAnnotations() { // propagate the primary annotation to the type variable because primary annotations // overwrite the upper and lower bounds of type variables when // getUpperBound/getLowerBound is called. - lowerBound = cowChild(lowerBound); + if (cowActive()) lowerBound = cowChild(lowerBound); if (lowerBound != null) { lowerBound.replaceAnnotations(newAnnos); } @@ -2429,7 +2434,7 @@ public AnnotatedTypeMirror getUpperBound() { freezeLazyComponent(upperBound); freezeLazyComponent(lowerBound); } - upperBound = cowChild(upperBound); + if (cowActive()) upperBound = cowChild(upperBound); return upperBound; } @@ -2745,7 +2750,7 @@ public AnnotatedTypeMirror getSuperBound() { freezeLazyComponent(superBound); freezeLazyComponent(extendsBound); } - this.superBound = cowChild(this.superBound); + if (cowActive()) this.superBound = cowChild(this.superBound); return this.superBound; } @@ -2780,7 +2785,7 @@ public AnnotatedTypeMirror getExtendsBound() { freezeLazyComponent(extendsBound); freezeLazyComponent(superBound); } - this.extendsBound = cowChild(this.extendsBound); + if (cowActive()) this.extendsBound = cowChild(this.extendsBound); return this.extendsBound; } @@ -2801,11 +2806,11 @@ void freezeComponents() { */ private void fixupBoundAnnotations() { if (!this.getAnnotationsField().isEmpty()) { - superBound = cowChild(superBound); + if (cowActive()) superBound = cowChild(superBound); if (superBound != null) { superBound.replaceAnnotations(this.getAnnotationsField()); } - extendsBound = cowChild(extendsBound); + if (cowActive()) extendsBound = cowChild(extendsBound); if (extendsBound != null) { extendsBound.replaceAnnotations(this.getAnnotationsField()); } @@ -2953,7 +2958,7 @@ public void addAnnotation(AnnotationMirror annotation) { public boolean removeAnnotation(AnnotationMirror a) { boolean ret = super.removeAnnotation(a); if (bounds != null) { - bounds = cowChildren(bounds); + if (cowActive()) bounds = cowChildren(bounds); for (AnnotatedTypeMirror bound : bounds) { ret |= bound.removeAnnotation(a); } @@ -2969,7 +2974,7 @@ private void fixupBoundAnnotations() { if (!this.getAnnotationsField().isEmpty()) { AnnotationMirrorSet newAnnos = this.getAnnotationsField(); if (bounds != null) { - bounds = cowChildren(bounds); + if (cowActive()) bounds = cowChildren(bounds); for (AnnotatedTypeMirror bound : bounds) { bound.replaceAnnotations(newAnnos); } @@ -3045,7 +3050,7 @@ public List getBounds() { fixupBoundAnnotations(); freezeLazyComponents(bounds); } - bounds = cowChildren(bounds); + if (cowActive()) bounds = cowChildren(bounds); return bounds; } @@ -3159,7 +3164,7 @@ public List getAlternatives() { alternatives = Collections.unmodifiableList(res); freezeLazyComponents(alternatives); } - alternatives = cowChildren(alternatives); + if (cowActive()) alternatives = cowChildren(alternatives); return alternatives; } From e49c813edb4cec2b0d0dd07faac923357711ee8e Mon Sep 17 00:00:00 2001 From: Werner Dietl Date: Tue, 23 Jun 2026 08:25:25 -0400 Subject: [PATCH 12/12] WIP: COW reference config (all 6 caches + gated); no downstream GC win Restore all six cache flips on top of the cowActive() gating, so this is the max-allocation-reduction reference (alloc -4.8%). Tested the "downstream / GC-bound timing win" hypothesis on all-systems 269 across a heap sweep. The wall penalty shrinks as the heap tightens (-Xmx512m +8.1%, 320m +0.5%, 256m +0.1%), BUT a clean GC measurement at 256m shows COW does NOT reduce GC: 0.77s vs 0.76s pauses, 225 collections both. So the gap-closing is measurement variance, not GC savings -- the -4.8% allocation does not convert to fewer collections. CF compilation is CPU-bound (~96% on-CPU, GC <=4% even at scale per the notes), so the allocation reduction has no wall payoff (GC ceiling ~4%, COW captures none of it) and cannot offset the +5% CPU tax. No single-compile or downstream timing win from COW. Co-Authored-By: Claude Opus 4.8 --- .../framework/type/AnnotatedTypeFactory.java | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java index a5f41e787d6d..89f0a2bd2cdc 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java @@ -1831,7 +1831,7 @@ public AnnotatedTypeMirror fromElement(Element elt) { if (shouldCache) { AnnotatedTypeMirror cached = elementCache.get(elt); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } if (elt.getKind() == ElementKind.PACKAGE) { @@ -1950,7 +1950,7 @@ private AnnotatedTypeMirror fromMember(Tree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromMemberTreeCache.get(tree); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } AnnotatedTypeMirror result = TypeFromTree.fromMember(this, tree); @@ -2046,7 +2046,7 @@ private AnnotatedTypeMirror fromExpression(ExpressionTree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromExpressionTreeCache.get(tree); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } @@ -2079,7 +2079,7 @@ private AnnotatedTypeMirror fromExpression(ExpressionTree tree) { if (shouldCache) { AnnotatedTypeMirror cached = fromTypeTreeCache.get(tree); if (cached != null) { - return cached.deepCopy(); + return AnnotatedTypeMirror.COW ? cached.cowCopy() : cached.deepCopy(); } } @@ -2850,7 +2850,10 @@ && shouldCacheMethodAsMemberOf() AnnotatedExecutableType cachedMethodType = cacheKey == null ? null : methodAsMemberOfCache.get(cacheKey); if (cachedMethodType != null) { - methodType = cachedMethodType.deepCopy(); + methodType = + AnnotatedTypeMirror.COW + ? (AnnotatedExecutableType) cachedMethodType.cowCopy() + : cachedMethodType.deepCopy(); } else { methodType = computeMethodTypeAsMemberOf(tree, methodElt, receiverType, inferTypeArgs); if (cacheKey != null) {