diff --git a/checker/src/main/java/org/checkerframework/checker/initialization/InitializationParentAnnotatedTypeFactory.java b/checker/src/main/java/org/checkerframework/checker/initialization/InitializationParentAnnotatedTypeFactory.java index 7836820929da..59f178a0b4cb 100644 --- a/checker/src/main/java/org/checkerframework/checker/initialization/InitializationParentAnnotatedTypeFactory.java +++ b/checker/src/main/java/org/checkerframework/checker/initialization/InitializationParentAnnotatedTypeFactory.java @@ -271,10 +271,15 @@ public AnnotationMirror createUnderInitializationAnnotation(TypeMirror typeFrame public @Nullable AnnotatedDeclaredType getSelfType(Tree tree) { AnnotatedDeclaredType selfType = super.getSelfType(tree); - if (assumeInitialized) { + if (assumeInitialized || selfType == null) { return selfType; } + // The loop below mutates the self type and its enclosing types + // (setSelfTypeInInitializationCode), so copy first: super.getSelfType may return a shared, + // immutable type from a cache. + selfType = selfType.deepCopy(); + TreePath path = getPath(tree); AnnotatedDeclaredType enclosing = selfType; while (path != null && enclosing != null) { diff --git a/checker/tests/nullness/ElementTypeCacheWildcardBound.java b/checker/tests/nullness/ElementTypeCacheWildcardBound.java new file mode 100644 index 000000000000..d98a30eb90b5 --- /dev/null +++ b/checker/tests/nullness/ElementTypeCacheWildcardBound.java @@ -0,0 +1,28 @@ +// Regression test: a (since-reverted) elementTypeCache "boundary flip" returned the shared +// frozen cache master from getAnnotatedType(Element) instead of a deep copy. A consumer then +// embedded a *sub-component* of that shared value -- here an unbounded wildcard's implicit +// java.lang.Object upper bound, derived from a JDK generic type's cached type-parameter bound -- +// into a fresh, non-frozen result type. Because the root was not frozen, the copy-on-frozen +// guards (which check only the root) did not copy it, and addComputedTypeAnnotations later +// mutated the frozen child, crashing with "Attempted to mutate a frozen AnnotatedTypeMirror with +// underlying type java.lang.Object". +// +// It needs a JDK parameterized type (whose annotated type is cached) used with an unbounded +// wildcard, plus a raw-generic-bounded type variable, in a generic method that is invoked so the +// dataflow transfer re-derives the local's type. Minimized from Guava's +// com.google.common.collect.SortedLists.binarySearch, which alltests did not cover. This test +// should type-check with no errors. + +import java.util.function.Function; + +public class ElementTypeCacheWildcardBound { + @SuppressWarnings("rawtypes") + static int f(Function fn) { + return g(fn); + } + + @SuppressWarnings("rawtypes") + static int g(Function fn) { + return 0; + } +} diff --git a/checker/tests/nullness/VarargCacheAliasing.java b/checker/tests/nullness/VarargCacheAliasing.java new file mode 100644 index 000000000000..4eb546fa1285 --- /dev/null +++ b/checker/tests/nullness/VarargCacheAliasing.java @@ -0,0 +1,19 @@ +// Regression test: AnnotatedTypeCopier.visitExecutable used to alias (rather than copy) the +// vararg type when deep-copying an executable type, so deepCopy() did not produce a fully +// independent type. The shared vararg subtree was then re-defaulted in place. This is benign +// while cached types are mutable, but corrupts a shared cached value once cache masters are +// frozen; with that enforcement it crashed with "Attempted to mutate a frozen +// AnnotatedTypeMirror". Calling JDK vararg methods (whose executable types are cached and +// re-defaulted) reproduces it. This test should type-check with no errors. + +import java.util.Arrays; +import java.util.List; + +public class VarargCacheAliasing { + void use(String s) throws Exception { + List xs = Arrays.asList(s, s, s); + List ys = Arrays.asList(s); + String f = String.format("%s %s", s, s); + java.lang.reflect.Method m = VarargCacheAliasing.class.getMethod("use", String.class); + } +} diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 71a8dff33fcd..02560ca0c602 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -108,6 +108,31 @@ thread-local map instead of allocating one per copy. Total allocation on large s dropped 14-19% (e.g. -19% on a 1500-method class) and about 7% on a many-file corpus; wall clock is roughly unchanged, the gain being reduced GC pressure. No user-visible behavior change. +`AnnotatedTypeMirror` has new `freeze()` and `isFrozen()` methods. Freezing a type +makes it (and every type reachable from it) reject primary-annotation changes, +throwing `BugInCF` on an attempted mutation; lazy initialization of bounds and type +arguments is still permitted. This lets the framework share an immutable cached type +without defensively deep-copying it. + +Fixed a latent aliasing bug in `AnnotatedTypeCopier`: when copying an executable +type, the copy shared the original's vararg type instead of copying it, so +`deepCopy()` did not produce a fully independent type. The annotated-type caches in +`AnnotatedTypeFactory` now freeze the value they store (the masters), so a future +in-place mutation of a cached type fails fast with `BugInCF` instead of silently +corrupting the shared value. + +`StructuralEqualityComparer.arePrimaryAnnosEqual` is now non-mutating. The Value +Checker's override previously normalized the two operands' annotations to a canonical +form by calling `replaceAnnotation` on them -- an equality check with a side effect on +its operands, which also prevents comparing a shared immutable type. It now computes +the canonical annotations and compares them without mutating the types. + +Performance: `getAnnotatedType(Tree)` for class and method declarations +(`classAndMethodTreeCache`) now returns the shared frozen cached type instead of a deep +copy on every hit; the few callers that mutate the result copy it first. This removes a +deep copy per cache hit for the read-only majority of callers. No user-visible behavior +change. + Fixed a bug that caused an IndexOutOfBoundsException for lambdas in varargs, for type systems that had the Aliasing Checker as a subchecker, like the Optional Checker. diff --git a/docs/developer/performance-notes.md b/docs/developer/performance-notes.md index ba0844dd2b96..bb45a64dc8b8 100644 --- a/docs/developer/performance-notes.md +++ b/docs/developer/performance-notes.md @@ -70,6 +70,80 @@ so small per-call wins paid back substantially. `AnnotatedNoType` fall through to the base, where the underlying `getKind()` is cheap. +### `AnnotatedTypeMirror` immutability foundation + +- **PR #1798** — *`freeze()` mechanism + `AnnotatedTypeCopier` vararg-aliasing fix + frozen cache + masters.* The foundation of the value-semantics program (full narrative under "AnnotatedTypeMirror + value-semantics program" in Short list). Adds a `frozen` bit to `AnnotatedTypeMirror`, a + `checkMutable()` guard on the three primary-annotation sinks (with + `AnnotationMirrorSet.makeUnmodifiable()` as a backstop), and a cycle-safe deep `freeze()` that freezes + only already-initialized components (lazy getters freeze what they create later). Freezes the master + stored at all eight `AnnotatedTypeFactory` caches, so a latent in-place mutation of a cached type now + fails fast with `BugInCF` instead of silently corrupting a shared value. Freezing flushed — and the PR + fixes — a real `AnnotatedTypeCopier.visitExecutable` bug: it aliased the original's vararg type into + the copy instead of copying it, so `deepCopy()` of an executable type was not fully independent. + Caches still `deepCopy()` on every hit, so this is **behavior-neutral and measured perf-neutral** + (deterministic allocation ±0.1% incl. a vararg-heavy workload; `freeze()` below the on-CPU sampling + threshold). Shipped for the enforced invariant and the bug fix, not a perf number. +- **PR #1798 (cont.) — `classAndMethodTreeCache` boundary flip (kept); `elementTypeCache` flip + (REVERTED).** The cross-cutting enabler was making `StructuralEqualityComparer.arePrimaryAnnosEqual` + non-mutating (the Value Checker's override used to normalize its operands in place, which both prevents + comparing a shared immutable type and is a side-effecting equality). Both post-pipeline caches were then + flipped to **return the shared frozen master instead of `deepCopy()`ing on every hit**, with the minority + of mutating callers copy-on-frozen at the mutation site. Measured win was modest: deterministic + `ThreadAllocationStatistics` (median of 3) **−0.75%** (Big300) / **−0.97%** (Big600) on generic-call code, + within noise elsewhere — ~1%, not the −5.3% an earlier estimate suggested (never reproduced against this + baseline; the copier was already cheap, see the post-mortem above). + **The `elementTypeCache` flip was then reverted: a full Guava nullness build (`test-guava.sh`, not covered + by `alltests`) crashed with `BugInCF` "Attempted to mutate a frozen AnnotatedTypeMirror with underlying + type java.lang.Object".** Root cause: a consumer lifts a *sub-component* of the shared frozen master — an + unbounded wildcard's implicit `Object` upper bound, derived from a JDK generic's cached type-parameter + bound (`Function` with `K extends Comparable`) — into a *fresh, non-frozen* result type; + `addComputedTypeAnnotations` then mutates the frozen child. The nine copy-on-frozen guards all copy at the + **root** (`if (type.isFrozen()) deepCopy()`), so a non-frozen root holding a frozen child slips through — + a hazard the root-level guard cannot catch and that escaped both `alltests` and the nine fixes. This is + **structural to returning a shared frozen value**: any path that reparents a child of the shared master is + a latent crash, unenumerable short of running every downstream project. For a ~1% win that is not worth + it; reverted (commit message references the Guava crash). Regression test: + `checker/tests/nullness/ElementTypeCacheWildcardBound.java` (minimized from + `com.google.common.collect.SortedLists.binarySearch`). **The `classAndMethodTreeCache` flip is kept** — it + is much lower-traffic (class/method declaration trees) and survived the full Guava build and `alltests` — + but it carries the same residual embedded-frozen-component risk in principle; **re-run downstream builds + (Guava et al.), not just `alltests`, before extending the shared-frozen-return pattern to any further + cache.** The lesson: a frozen-master *tripwire* that still returns `deepCopy()` is safe (the master is + never handed out); *returning* the shared frozen value is what creates the reparenting hazard. + **Can the flip be salvaged? Three options, with a cost ladder (the answer is "not without copy-on-write"):** + (1) *Fix each reparenting site* (the nine copy-on-frozen guards, plus a tenth for this wildcard/type-var + bound). Cheap per site, but the obvious construction sites (`BoundsInitializer`, the wildcard visitor) + already build fresh, so the frozen child enters through a subtler path; the guarantee needed ("nothing + ever reparents a child of a shared frozen master") is a convention, not enforced — Guava found what + `alltests` + nine fixes missed, and the next codebase could find an eleventh. Not shippable for ~1%. + (2) *Deep guard at the choke point* — `deepCopy()` if **any** node is frozen, not just the root. Complete + for the choke-point mutator, but the scan cost scales with the type and frozen children appear whenever a + type embeds a cached generic bound (common), so it copies about as often as today and likely erases the + ~1% — net-neutral-to-negative; measurement-gated, unpromising. (3) *Copy-on-write ATMs* — a frozen node's + mutators return a fresh shallow node instead of throwing, so sharing is safe regardless of who reparents + what; the whole bug class disappears. **This is the only complete fix, and it would make all eight caches + flippable, not just `elementType` — so its payoff is the combined copy elimination, not ~1%.** It is a + separate, measured architectural project (see "the recommended next direction" in the Short list and the + copy-on-write notes below), not a patch to this PR. Verdict: keep the flip reverted; pursue the allocation + win, if at all, via copy-on-write as its own effort. +- **Post-mortem: why the immutability allocation win came in at ~1%, not the projected large payoff.** The + program was motivated by an earlier profile attributing `AnnotatedTypeCopier.visit` ~2% on-CPU self-time + and **the dominant share of `Object[]` TLAB allocation (~22%)**. A fresh full-`checknullness` trace taken + *after* this PR (11.8k `ExecutionSample`s, 156 s span) shows that figure is **stale**: `AnnotatedTypeCopier` + is now **~0.76% self-time and ~1-1.5% of allocation**. The intervening work — the PR #1777 + `methodAsMemberOf`/`directSupertypes`/`elementType` caches, the thread-local copier `originalToCopy` map, + and lazy `AnnotatedTypeScanner.visitedNodes` — had already harvested most of the copier allocation the + immutability program was meant to remove. So by the time the boundary flips landed there was little copier + cost left to delete, and the flip removes only the per-hit copy for the read-only-majority of consumers + (≈1%). **Lesson: re-trace the current baseline before committing to an architectural plan built on an + older profile — an allocation hotspot named in this log may already have been shrunk by later commits.** + The current `checknullness` CPU profile is **flat** (hottest leaf `IdentityHashMap.get` at 2.98%, spread + across ~10 callers); the largest remaining *addressable* allocation slices are `AnnotatedTypeScanner.markVisited`'s + per-scan `IdentityHashMap` (~5% of allocation) and `AnnotationMirrorSet` construction+iteration (~6-10%), + each a careful per-item job with low-single-digit wall-clock upside, not a large lever. + ### `AnnotationMirrorSet` and annotation utilities - **PR #1649** — *Reimplement `AnnotationMirrorSet` using an @@ -896,6 +970,40 @@ Bring new evidence before revisiting any of these — a JFR trace on a workload not previously considered, or a measurement that contradicts the prior finding. A fresh hypothesis is not new evidence. +- **Cache-boundary flips after freezing the masters — PARTIALLY SUPERSEDED (PR #1798).** The first cut + rejected boundary flips wholesale: "the cache-return copy is load-bearing; the dominant consumers + mutate the returned type, so a flip only moves the copy." **That was survivorship bias from the + `BugInCF` flush** — the flush only enumerates the *mutating* consumers, not the read-only majority, so + reasoning from it overcounts the cost. A direct measurement of the read-only fraction (65–88% for + `getAnnotatedType(Element)`) and a deterministic allocation A/B then showed the **`elementTypeCache` + and `classAndMethodTreeCache` flips DO pay (small): ~−1% on generic-call-heavy code, noise elsewhere** + (NOT the −5.3% an earlier estimate suggested — never reproduced against the post-flip baseline). Both + shipped in PR #1798 (see the foundation section). The lesson stands for three *other* flips that were + tried and genuinely do not pay, because their consumer is *always* mutating: (a) the raw **Element + boundary feeding the tree pipeline** — `TypeFromExpressionVisitor` → `addComputedTypeAnnotations` + (defaulting + flow refinement + annotators) rewrites the whole type every time; (b) the **`methodFromUse` + on-hit copy-elision** — type-argument inference (`typeinference8.Resolution.resolveWithLowerBounds`) + mutates the method type in place; and (c) the **`directSupertypesCache` flip** — its dominant consumer is + `AsSuperVisitor` (the cache exists *because* `asSuper`/`allSupertypes` recompute supertypes constantly), + which mutates each returned supertype in place (`copyPrimaryAnnos`, `setUpperBound`, `fixupBoundAnnotations` + via `visitDeclared_{Typevar,Wildcard,Intersection}`); flipping it (return the frozen masters, skip the + per-hit `deepCopySupertypes`) flushed 60 `BugInCF`s, all in `AsSuperVisitor`, and fixing them would + relocate the per-hit copy into `AsSuperVisitor` — fired once per asSuper walk-step, the same frequency — + a provable wash. Reverted. Rule of thumb confirmed: **a post-pipeline cache whose hits are mostly + read-only pays from a flip; a cache whose hot consumer rewrites the result in place does not — and you + can tell which from whether the hot consumer (tree pipeline / inference / asSuper) is in the flush.** +- **Caching `AnnotatedTypeMirror.hashCode()` on frozen types (PR #1798 session).** The standing idea + (the hash can't be cached *because* ATMs are mutable) is unblocked for frozen types — but + instrumentation showed **0.0% of `hashCode()` calls land on frozen types** (every hot hash target is a + mutable working copy, since the caches hand out copies). Worthless in the current architecture, and it + would only become useful after a boundary flip that itself does not pay. +- **Shallow-location defaulting shortcut (PR #1798 session).** Skipping `QualifierDefaults`'s recursive + descent for top-level-only locations (FIELD/PARAMETER/RETURN/RECEIVER/RESOURCE_VARIABLE/ + EXCEPTION_PARAMETER/CONSTRUCTOR_RESULT) is sound and cut scan calls **10.2%**, but those saved scans are + over cheap shallow types and **allocation was flat** — negligible. The cost is the deep `OTHERWISE`/ + bound traversals over generic types; merging those into a single pass is a high-risk refactor with a + ~2% ceiling (defaulting is single-digit-% of CPU). Distinct from the deferred Defaulting Phase 2 + (caching the result); this was the cache-free variant. - **`AnnotationMirror → QualifierKind` second-level cache in qualifier hierarchies (June 2026).** `NoElementQualifierHierarchy.getQualifierKind(AnnotationMirror)` and the matching method in `ElementQualifierHierarchy` already use an `elementToQualifierKind: IdentityHashMap[]`/`LinkOption[]` and everything reachable — +which is why every flushed underlying type was an array or an array-subtree node). Defaulting then +mutated that shared subtree. Fixed with `copy.setVarargType((AnnotatedArrayType) visit(original.getVarargType(), +originalToCopy))` — the `originalToCopy` map returns the already-made parameter copy when the vararg is +the last parameter (the common case, so the fix is allocation-neutral), else a fresh copy. With that one +fix, freezing all eight masters is green on the full suite. **Lesson: when freezing flushes a cluster of +mutations, look for a shared copier/construction bug before assuming pervasive aliasing.** + +*The aliasing was benign to results — the only symptom is the freeze crash.* The vararg type is consumed +read-only (`PropagationTreeAnnotator`, `BaseTypeVisitor`); its only post-copy mutator is qualifier +defaulting, which is idempotent (`addMissingAnnotation`), so the shared subtree always got the same +annotations and no wrong diagnostic ever resulted — which is why it was latent and master's suite was +green. Confirmed three ways on one program (JDK vararg calls `Arrays.asList`/`String.format`/`Class.getMethod`): +clean on master, `BugInCF` on freeze-without-fix, clean on freeze+fix. Consequence: the regression test +(`checker/tests/nullness/VarargCacheAliasing.java`, PR #1798) only demonstrates the bug *with* the freeze +enforcement present; a standalone fix would need a unit test asserting `deepCopy` independence. This is +also why PR #1798 keeps the fix and the freeze work in one change. + +*The load-bearing-copy finding — four attempts, all confirming the cache-return `deepCopy` cannot just be +dropped.* The whole point of freezing masters was to then return the shared frozen instance and delete the +copy. It does not work, because the dominant consumers mutate what they get back: +- **Element-boundary flip** (`getAnnotatedType(Element)` returns the frozen master): flushed + `DefaultInferredTypesApplier` (flow refinement, 60), `constructorFromUse` (`type = getAnnotatedType(elt); + type.clearAnnotations()`, 25), `CommitmentTypeAnnotator`, `DefaultQualifierPolymorphism`, `ValueTreeAnnotator`, + ... The results feed the always-mutating tree pipeline (`visitIdentifier`/`visitMemberSelect`/`asMemberOf` + → `addComputedTypeAnnotations`), so fixing each site means a `deepCopy()` *before* the mutation — which + **moves** the copy to the consumer, not removes it. The flip saves a copy only for read-only direct + consumers, the minority. +- **`methodFromUse` copy-elision** (skip the on-hit `deepCopy` since `typeVarSubstitutor.substitute` copies + again for generic methods): **type-argument inference mutates the method type in place** — + `findTypeArguments` → `DefaultTypeArgumentInference.inferTypeArgs` → + `typeinference8.Resolution.resolveWithLowerBounds` calls `replaceAnnotations` on a component of `preType`. + So the pre-inference copy is load-bearing. +- **`hashCode` caching on frozen ATMs** (the perf-notes' standing "can't cache because mutable" item): + instrumented **0.0% of `hashCode()` calls land on frozen types** (0 of 185k / 370k on the size sweep) — + every hot hash target is a mutable working copy, because the caches return copies. Worthless in the + current architecture, and doubly blocked (it would need the boundary flip, which itself does not pay). +- **Shallow-location defaulting shortcut** (skip the recursive descent for the top-level-only locations + FIELD/PARAMETER/RETURN/RECEIVER/RESOURCE_VARIABLE/EXCEPTION_PARAMETER/CONSTRUCTOR_RESULT — but NOT + LOCAL_VARIABLE, which has a type-variable-use special case): cut scan calls only **10.2%** (586k→527k), + and those saved scans are over cheap shallow types (`Object` parameters); **allocation flat**. The + expensive defaulting is the deep `OTHERWISE` + bound-location traversals over generic types, which the + shortcut does not touch. (Separately measured: `addMissingAnnotation` is **74% no-op**, and + `applyDefaultsElement` does N full scans, one per `Default` — so a *full* single-pass merge could pay, + but it is a high-risk refactor of the recursive bound logic with a ~2% ceiling.) + +**Where this leaves the program.** A boundary flip *is* achievable — but it relocates the copy to each +mutating consumer rather than removing it, so the realized win is only the read-only-consumer fraction. +Two findings refine the earlier "blocked" verdict: + +- **The cross-cutting blocker was one latent bug, now fixed (PR #1798): side-effecting equality.** + `ValueAnnotatedTypeFactory`'s `arePrimaryAnnosEqual` override normalized its operands by mutating them + (`replaceAnnotation`) before comparing. That fired during *every* cache flip (it runs in subtyping/equality, + which all cache results flow through). Made non-mutating (compute the canonical annotations, compare without + mutating). It is the prerequisite for any flip and a correctness fix on its own. +- **`classAndMethodTreeCache` flip shipped (PR #1798) — green, but modest.** With the equality fix plus + copy-on-frozen at the ~6 mutating consumers it flushed (`getMethodReturnType`, `getSelfType`, the + `getAnnotatedType(Tree)` pipeline choke-point, `constructorFromUse`'s enclosing type, and + `ValueVisitor.checkOverride`), the flip is green on the full suite. **But deterministic A/B is ~−1% on a + method-heavy file and ~0% on realistic code — `classAndMethodTreeCache` is low-volume.** Shipped for + GC-relief + to establish the copy-on-frozen consumer-fix pattern. +- **The high-volume `elementTypeCache` is mutation-dominated, so likely also modest.** Its flip flushed + 108 events; its dominant consumer is `asMemberOf` (every method call via `methodFromUse`), which mutates the + result (poly resolution, substitution, `postAsMemberOf`). Flipping it needs `asMemberOf` to copy-on-frozen + on its alias-return paths, which moves the copy back — limiting the win to read-only element-type queries. + Not pursued: large fix set, likely-modest win. + +So the per-cache lesson: the flip is *mechanically* unblockable (copy-on-frozen at the enumerated mutating +consumers; the freeze tripwire makes it incrementally safe), but the high-volume caches' hot consumers mutate, +so the realized win is small. The larger allocation win still needs **copy-on-write** (mutator returns a fresh +node sharing unchanged children — though the whole-tree re-annotators like defaulting/flow get no benefit) or +**eliminating redundant re-annotation** (Defaulting Phase 2). Prototype + JFR-A/B before more flips. The +higher-leverage perf target remains CF→javac internals (see the open venues). + #### Open venues (current — global trace after the smaller-scope `declarationFromElement` scan) A fresh full-`checknullness` trace (11,009 on-CPU samples; javac internals 35.8% / type factory 34.6%) has a **flat leaf profile (no leaf > ~3%)** — the per-leaf hot spots are mined out. Remaining CF-controllable clusters and their state, highest-leverage first: -1. **Immutability program (recommended next).** The deep-copy tax (`AnnotatedTypeCopier` ~2% self-time - + dominant `Object[]` allocation share) is what the shipped caches pay and is the +50–70 MB retained - heap. The one lever the evidence positively supports; large/staged. See the narrative header above. +1. **Immutability program — foundation + first flip shipped (PR #1798); remaining win small per cache.** + Shipped: the `freeze()` mechanism, the `AnnotatedTypeCopier` vararg-aliasing fix, freezing all eight + cache masters, the non-mutating-equality fix, and the first boundary flip (`classAndMethodTreeCache` + returns the shared frozen value, with copy-on-frozen at its mutating consumers). The flip is **green but + ~−1%/~0%** (low-volume cache). The flip technique is mechanically unblockable (copy-on-frozen at the + enumerated mutating consumers), but the high-volume caches' dominant consumers mutate (`elementTypeCache` + → `asMemberOf`), so their realized win is also likely modest. The larger win needs copy-on-write or + eliminating redundant re-annotation, *not* more boundary flips — see the narrative ("Where this leaves + the program") and Tried and rejected. Re-open with a copy-on-write prototype, measured. 2. **Defaulting Phase 2 (tree-path memoization).** Measured 88% `(scope, type)` repeat on the tree path, ~9.3 scans/call; per-CU clearing bounds the memory. *Gate on a within-CU-repeat measurement first*, and note it carries the same write-back tax that sank the `constructorFromUse` cache (real flat-risk). + PR #1798 also measured the cheaper cache-free variant (a "shallow-location" shortcut) and found it + negligible — see Tried and rejected; the deep `OTHERWISE`/bound traversals are where the cost is, and + merging those is the risky part. 3. **`getPath` / TreePath construction (~3.2%) — largely addressed by PR #1786 + #1788.** 68% of `TreePath.` was under `AnnotatedTypeFactory.getPath`'s slow path (uncached `TreePath.getPath(root, tree)` scans on cache miss + heuristic failure). PR #1786 caches the diff --git a/framework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java b/framework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java index 09bb60767911..076a9a7f7c19 100644 --- a/framework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java +++ b/framework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java @@ -401,16 +401,19 @@ public StructuralEqualityComparer createEqualityComparer() { @Override protected boolean arePrimaryAnnosEqual( AnnotatedTypeMirror type1, AnnotatedTypeMirror type2) { - type1.replaceAnnotation( + // Normalize equivalent qualifiers (e.g. @IntRange(1,1) and @IntVal(1)) to a + // canonical form before comparing, but do NOT mutate type1/type2: compute + // the canonical annotations and compare them directly. The types may be + // shared frozen cache values, so a mutating comparison would corrupt them. + AnnotationMirror anno1 = convertToUnknown( convertSpecialIntRangeToStandardIntRange( - type1.getAnnotationInHierarchy(UNKNOWNVAL)))); - type2.replaceAnnotation( + type1.getAnnotationInHierarchy(UNKNOWNVAL))); + AnnotationMirror anno2 = convertToUnknown( convertSpecialIntRangeToStandardIntRange( - type2.getAnnotationInHierarchy(UNKNOWNVAL)))); - - return super.arePrimaryAnnosEqual(type1, type2); + type2.getAnnotationInHierarchy(UNKNOWNVAL))); + return arePrimaryAnnosEqual(anno1, anno2, type1, type2); } }; } diff --git a/framework/src/main/java/org/checkerframework/common/value/ValueVisitor.java b/framework/src/main/java/org/checkerframework/common/value/ValueVisitor.java index 4a1b24d0c629..17ef2f6359a1 100644 --- a/framework/src/main/java/org/checkerframework/common/value/ValueVisitor.java +++ b/framework/src/main/java/org/checkerframework/common/value/ValueVisitor.java @@ -113,6 +113,14 @@ protected boolean checkOverride( AnnotatedTypeMirror.AnnotatedExecutableType overridden, AnnotatedTypeMirror.AnnotatedDeclaredType overriddenType) { + // replaceSpecialIntRangeAnnotations mutates the executable types in place; they may be + // shared frozen cache values (from getAnnotatedType on the method trees), so copy first. + if (overrider.isFrozen()) { + overrider = overrider.deepCopy(); + } + if (overridden.isFrozen()) { + overridden = overridden.deepCopy(); + } replaceSpecialIntRangeAnnotations(overrider); replaceSpecialIntRangeAnnotations(overridden); diff --git a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeCopier.java b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeCopier.java index fa45a672c608..81e63d14245c 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeCopier.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeCopier.java @@ -236,7 +236,13 @@ public AnnotatedTypeMirror visitExecutable( } if (original.getVarargType() != null) { - copy.setVarargType(original.getVarargType()); + // Copy (do not alias) the vararg type. If the vararg type is the last parameter type + // (the usual case, before adaptParameters expands it), originalToCopy already maps it + // to its copy, so visit() returns that copy and the structure is preserved; otherwise + // it is freshly copied. Aliasing the original's vararg type here would let two + // "independent" copies share a subtree. + copy.setVarargType( + (AnnotatedArrayType) visit(original.getVarargType(), originalToCopy)); } else { copy.computeVarargType(); } 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 1c42ebda6fae..2dcfe26e20bb 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeFactory.java @@ -1528,7 +1528,7 @@ public AnnotatedTypeMirror getAnnotatedType(Element elt) { AnnotatedTypeMirror type = fromElement(elt); addComputedTypeAnnotations(elt, type); if (useCache) { - elementTypeCache.put(elt, type.deepCopy()); + elementTypeCache.put(elt, frozenDeepCopy(type)); } return type; } @@ -1579,7 +1579,9 @@ public AnnotatedTypeMirror getAnnotatedType(Tree tree) { if (shouldCache) { AnnotatedTypeMirror cached = classAndMethodTreeCache.get(tree); if (cached != null) { - return cached.deepCopy(); + // The cached (post-pipeline) type is frozen and shared without copying; callers + // that mutate the result must deepCopy() it first. + return cached; } } @@ -1598,6 +1600,12 @@ public AnnotatedTypeMirror getAnnotatedType(Tree tree) { + tree.getKind()); } + // The from* result can be a shared frozen cache value (e.g. an expression whose type is a + // class/method type served from classAndMethodTreeCache); addComputedTypeAnnotations + // mutates the type, so copy it first when frozen. + if (type.isFrozen()) { + type = type.deepCopy(); + } addComputedTypeAnnotations(tree, type); if (tree instanceof TypeCastTree) { type = applyCaptureConversion(type); @@ -1605,7 +1613,7 @@ public AnnotatedTypeMirror getAnnotatedType(Tree tree) { if (shouldCache && (isClassTree || tree instanceof MethodTree)) { // Don't cache VARIABLE - classAndMethodTreeCache.put(tree, type.deepCopy()); + classAndMethodTreeCache.put(tree, frozenDeepCopy(type)); } else { // No caching otherwise } @@ -1821,7 +1829,7 @@ public AnnotatedTypeMirror fromElement(Element elt) { && !stubTypes.isParsing() && !ajavaTypes.isParsing() && (currentFileAjavaTypes == null || !currentFileAjavaTypes.isParsing())) { - elementCache.put(elt, type.deepCopy()); + elementCache.put(elt, frozenDeepCopy(type)); } return type; } @@ -1907,7 +1915,7 @@ private AnnotatedTypeMirror fromMember(Tree tree) { } if (shouldCache) { - fromMemberTreeCache.put(tree, result.deepCopy()); + fromMemberTreeCache.put(tree, frozenDeepCopy(result)); } return result; @@ -1994,7 +2002,7 @@ private AnnotatedTypeMirror fromExpression(ExpressionTree tree) { && !(tree instanceof NewClassTree) && !(tree instanceof NewArrayTree) && !(tree instanceof ConditionalExpressionTree)) { - fromExpressionTreeCache.put(tree, result.deepCopy()); + fromExpressionTreeCache.put(tree, frozenDeepCopy(result)); } return result; } @@ -2022,7 +2030,7 @@ private AnnotatedTypeMirror fromExpression(ExpressionTree tree) { AnnotatedTypeMirror result = TypeFromTree.fromTypeTree(this, tree); if (shouldCache) { - fromTypeTreeCache.put(tree, result.deepCopy()); + fromTypeTreeCache.put(tree, frozenDeepCopy(result)); } return result; } @@ -2781,7 +2789,7 @@ && shouldCacheMethodAsMemberOf() } else { methodType = computeMethodTypeAsMemberOf(tree, methodElt, receiverType, inferTypeArgs); if (cacheKey != null) { - methodAsMemberOfCache.put(cacheKey, methodType.deepCopy()); + methodAsMemberOfCache.put(cacheKey, frozenDeepCopy(methodType)); } } List typeargs = new ArrayList<>(methodElt.getTypeParameters().size()); @@ -3026,7 +3034,11 @@ public List getDirectSupertypes(AnnotatedDeclaredType typ } List result = SupertypeFinder.directSupertypes(type); if (key != null) { - directSupertypesCache.put(key, deepCopySupertypes(result)); + List masters = deepCopySupertypes(result); + for (int i = 0, n = masters.size(); i < n; ++i) { + masters.get(i).freeze(); + } + directSupertypesCache.put(key, masters); } return Collections.unmodifiableList(result); } @@ -3046,6 +3058,24 @@ private List deepCopySupertypes(List the type of {@code type} + * @param type the type to copy and freeze + * @return a frozen deep copy of {@code type} + */ + private static T frozenDeepCopy(T type) { + @SuppressWarnings("unchecked") // deepCopy() preserves the runtime type + T copy = (T) type.deepCopy(); + copy.freeze(); + return copy; + } + /** * Key for {@link #directSupertypesCache}: a declared type compared structurally via {@link * #structuralComparer} (hashed by {@code AnnotatedTypeMirror.hashCode}; a hash mismatch only @@ -3387,6 +3417,11 @@ protected ParameterizedExecutableType constructorFromUse( // Get the enclosing type of the constructor, if one exists. // this.new InnerClass() AnnotatedDeclaredType enclosingType = (AnnotatedDeclaredType) getReceiverType(tree); + if (enclosingType != null && enclosingType.isFrozen()) { + // getReceiverType may return a shared frozen cache value; it is embedded in `type` and + // mutated by addComputedTypeAnnotations below, so copy it first. + enclosingType = enclosingType.deepCopy(); + } type.setEnclosingType(enclosingType); // Add computed annotations to the type. @@ -3590,6 +3625,11 @@ protected void constructorFromUsePreSubstitution( public AnnotatedTypeMirror getMethodReturnType(MethodTree m) { AnnotatedExecutableType methodType = getAnnotatedType(m); AnnotatedTypeMirror ret = methodType.getReturnType(); + // getAnnotatedType(m) may be a shared frozen cache value; callers of this method (and its + // overrides) mutate the returned type, so hand back a mutable copy in that case. + if (ret.isFrozen()) { + ret = ret.deepCopy(); + } return ret; } 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 f19239f33e64..aa56faa2d810 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java +++ b/framework/src/main/java/org/checkerframework/framework/type/AnnotatedTypeMirror.java @@ -99,6 +99,20 @@ public abstract class AnnotatedTypeMirror implements DeepCopyableLazy initialization of structural components (type-variable and wildcard bounds, type + * arguments, array component types, executable-type parts) is still permitted on a frozen type; + * the freshly created components are frozen as they are produced, so the immutability of the + * whole reachable graph is preserved. + */ + private boolean frozen = false; + // /** The explicitly written annotations on this type. */ // TODO: use this to cache the result once computed? For generic types? // protected final AnnotationMirrorSet explicitannotations = @@ -649,6 +663,7 @@ public void addAnnotation(AnnotationMirror annotation) { if (annotation == null) { throw new BugInCF("AnnotatedTypeMirror.addAnnotation: null argument."); } + checkMutable(); if (atypeFactory.isSupportedQualifier(annotation)) { this.primaryAnnotations.add(annotation); } else { @@ -801,6 +816,7 @@ public void replaceAnnotations(AnnotationMirrorSet replAnnos) { */ // typetools removePrimaryAnnotation public boolean removeAnnotation(AnnotationMirror a) { + checkMutable(); AnnotationMirror anno = AnnotationUtils.getSame(primaryAnnotations, a); if (anno != null) { return primaryAnnotations.remove(anno); @@ -875,9 +891,96 @@ public boolean removeAnnotations(Iterable annotation /** Removes all primary annotations on this type. */ // typetools: clearPrimaryAnnotations public void clearAnnotations() { + checkMutable(); primaryAnnotations.clear(); } + /** + * Throws {@link BugInCF} if this type is {@linkplain #freeze() frozen}. Called by the primary + * annotation mutators before they change anything, so that a frozen (shared) type cannot be + * corrupted in place. Callers that need to change a frozen type must {@link #deepCopy()} it + * first and mutate the copy. + */ + protected final void checkMutable() { + if (frozen) { + throw new BugInCF( + "Attempted to mutate a frozen AnnotatedTypeMirror with underlying type %s." + + " Call deepCopy() before mutating a type obtained from a cache.", + underlyingType); + } + } + + /** + * Returns true if this type has been {@linkplain #freeze() frozen}. + * + * @return true if this type is frozen + */ + public final boolean isFrozen() { + return frozen; + } + + /** + * Freezes this type and every type reachable from it that has already been initialized, making + * them effectively immutable: subsequent attempts to change their primary annotations throw + * {@link BugInCF}. Idempotent. + * + *

This does not force lazy initialization of uninitialized components + * (type-variable and wildcard bounds, type arguments, etc.); doing so would be expensive and is + * unnecessary. Instead, the lazy getters freeze any component they create while this type is + * frozen, so the reachable graph stays fully frozen without eagerly materializing it. + * + *

Cycles (for example a recursive type variable whose bound refers back to itself) terminate + * because {@link #frozen} is set before {@link #freezeComponents()} recurses: a re-entry on a + * type already being frozen returns immediately. + */ + public final void freeze() { + if (frozen) { + return; + } + frozen = true; + // Reject any further direct mutation of the annotation set, including via + // getAnnotationsField() and the AnnotatedDeclaredTypeNoHierarchy.addAnnotation bypass. + primaryAnnotations.makeUnmodifiable(); + freezeComponents(); + } + + /** + * Freezes the already-initialized structural components of this type (bounds, type arguments, + * component types, etc.). Overridden by each composite subclass; the base implementation has no + * components to freeze. Only components that have already been initialized are frozen; lazy + * getters take care of components created later (see {@link #freeze()}). + */ + void freezeComponents() { + // No components in the base class. + } + + /** + * If this type is frozen, freezes {@code component} (when non-null). Used by lazy getters to + * keep the reachable graph frozen when a component is materialized after this type was frozen. + * + * @param component a structural component of this type, or null + */ + final void freezeLazyComponent(@Nullable AnnotatedTypeMirror component) { + if (frozen && component != null) { + component.freeze(); + } + } + + /** + * If this type is frozen, freezes each element of {@code components} (when non-null). Used by + * lazy getters to keep the reachable graph frozen when components are materialized after this + * type was frozen. + * + * @param components structural components of this type, or null + */ + final void freezeLazyComponents(@Nullable List components) { + if (frozen && components != null) { + for (int i = 0, n = components.size(); i < n; ++i) { + components.get(i).freeze(); + } + } + } + @SideEffectFree @Override public final String toString() { @@ -1254,9 +1357,22 @@ public List getTypeArguments() { ++i; } } + freezeLazyComponents(typeArgs); return typeArgs; } + @Override + void freezeComponents() { + if (enclosingType != null) { + enclosingType.freeze(); + } + if (typeArgs != null) { + for (int i = 0, n = typeArgs.size(); i < n; ++i) { + typeArgs.get(i).freeze(); + } + } + } + /** * Returns true if the underlying type is raw. The receiver of this method is not raw, * however; its annotated type arguments have been inferred. @@ -1533,6 +1649,7 @@ public List getParameterTypes() { } setParameterTypes(Collections.unmodifiableList(newParamTypes)); } + freezeLazyComponents(paramTypes); } // No need to copy or wrap; it is an unmodifiable list. return paramTypes; @@ -1642,6 +1759,7 @@ public AnnotatedTypeMirror getReturnType() { returnType = createType(aret, atypeFactory, false); } returnTypeComputed = true; + freezeLazyComponent(returnType); } return returnType; } @@ -1691,6 +1809,7 @@ public AnnotatedTypeMirror getReturnType() { receiverType = (AnnotatedDeclaredType) type; } receiverTypeComputed = true; + freezeLazyComponent(receiverType); } return receiverType; } @@ -1736,6 +1855,7 @@ public List getThrownTypes() { } setThrownTypes(Collections.unmodifiableList(newThrownTypes)); } + freezeLazyComponents(thrownTypes); } // No need to copy or wrap; it is an unmodifiable list. return thrownTypes; @@ -1784,11 +1904,35 @@ public List getTypeVariables() { } setTypeVariables(Collections.unmodifiableList(newTypeVarTypes)); } + freezeLazyComponents(typeVarTypes); } // No need to copy or wrap; it is an unmodifiable list. return typeVarTypes; } + @Override + void freezeComponents() { + // Freeze only already-computed components; do not force lazy initialization. + if (paramTypesComputed) { + freezeLazyComponents(paramTypes); + } + if (varargType != null) { + varargType.freeze(); + } + if (receiverTypeComputed && receiverType != null) { + receiverType.freeze(); + } + if (returnTypeComputed && returnType != null) { + returnType.freeze(); + } + if (thrownTypesComputed) { + freezeLazyComponents(thrownTypes); + } + if (typeVarTypesComputed) { + freezeLazyComponents(typeVarTypes); + } + } + @Override public AnnotatedExecutableType deepCopy(boolean copyAnnotations) { return (AnnotatedExecutableType) new AnnotatedTypeCopier(copyAnnotations).visit(this); @@ -1931,10 +2075,18 @@ public AnnotatedTypeMirror getComponentType() { ((ArrayType) underlyingType).getComponentType(), atypeFactory, false)); + freezeLazyComponent(componentType); } return componentType; } + @Override + void freezeComponents() { + if (componentType != null) { + componentType.freeze(); + } + } + @Override public AnnotatedArrayType deepCopy(boolean copyAnnotations) { return (AnnotatedArrayType) new AnnotatedTypeCopier(copyAnnotations).visit(this); @@ -2117,6 +2269,8 @@ public AnnotatedTypeMirror getLowerBound() { if (lowerBound == null) { // lazy init BoundsInitializer.initializeBounds(this); fixupBoundAnnotations(); + freezeLazyComponent(lowerBound); + freezeLazyComponent(upperBound); } return lowerBound; } @@ -2188,14 +2342,38 @@ public AnnotatedTypeMirror getUpperBound() { if (upperBound == null) { // lazy init BoundsInitializer.initializeBounds(this); fixupBoundAnnotations(); + freezeLazyComponent(upperBound); + freezeLazyComponent(lowerBound); } return upperBound; } + @Override + void freezeComponents() { + if (upperBound != null) { + upperBound.freeze(); + } + if (lowerBound != null) { + lowerBound.freeze(); + } + } + + /** + * Returns the upper and lower bounds of this type variable, forcing their lazy + * initialization if necessary. + * + * @return the bounds of this type variable + */ public AnnotatedTypeParameterBounds getBounds() { return new AnnotatedTypeParameterBounds(getUpperBound(), getLowerBound()); } + /** + * Returns the upper and lower bound fields of this type variable directly, without forcing + * lazy initialization (the fields may be null). + * + * @return the bound fields of this type variable + */ public AnnotatedTypeParameterBounds getBoundFields() { return new AnnotatedTypeParameterBounds(getUpperBoundField(), getLowerBoundField()); } @@ -2479,6 +2657,8 @@ public AnnotatedTypeMirror getSuperBound() { if (superBound == null) { BoundsInitializer.initializeBounds(this); fixupBoundAnnotations(); + freezeLazyComponent(superBound); + freezeLazyComponent(extendsBound); } return this.superBound; } @@ -2511,10 +2691,27 @@ public AnnotatedTypeMirror getExtendsBound() { if (extendsBound == null) { BoundsInitializer.initializeBounds(this); fixupBoundAnnotations(); + freezeLazyComponent(extendsBound); + freezeLazyComponent(superBound); } return this.extendsBound; } + @Override + void freezeComponents() { + if (extendsBound != null) { + extendsBound.freeze(); + } + if (superBound != null) { + superBound.freeze(); + } + } + + /** + * Copies this wildcard's primary annotations onto its extends and super bounds, replacing + * any existing annotations in the same hierarchy. Has no effect if this wildcard has no + * primary annotations or the bounds are not yet initialized. + */ private void fixupBoundAnnotations() { if (!this.getAnnotationsField().isEmpty()) { if (superBound != null) { @@ -2755,10 +2952,16 @@ public List getBounds() { (TypeMirror bnd) -> createType(bnd, atypeFactory, false), ubounds); bounds = Collections.unmodifiableList(res); fixupBoundAnnotations(); + freezeLazyComponents(bounds); } return bounds; } + @Override + void freezeComponents() { + freezeLazyComponents(bounds); + } + /** * Sets the bounds. * @@ -2862,9 +3065,15 @@ public List getAlternatives() { createType(alt, atypeFactory, false), ualts); alternatives = Collections.unmodifiableList(res); + freezeLazyComponents(alternatives); } return alternatives; } + + @Override + void freezeComponents() { + freezeLazyComponents(alternatives); + } } /** diff --git a/framework/src/main/java/org/checkerframework/framework/type/StructuralEqualityComparer.java b/framework/src/main/java/org/checkerframework/framework/type/StructuralEqualityComparer.java index 516450944b39..0f218741c2af 100644 --- a/framework/src/main/java/org/checkerframework/framework/type/StructuralEqualityComparer.java +++ b/framework/src/main/java/org/checkerframework/framework/type/StructuralEqualityComparer.java @@ -110,17 +110,40 @@ public boolean areEqualInHierarchy( * @return true if type1 and type2 have the same set of annotations */ protected boolean arePrimaryAnnosEqual(AnnotatedTypeMirror type1, AnnotatedTypeMirror type2) { - if (currentTop != null) { - AnnotationMirror anno1 = type1.getAnnotationInHierarchy(currentTop); - AnnotationMirror anno2 = type2.getAnnotationInHierarchy(currentTop); - TypeMirror typeMirror1 = type1.underlyingType; - TypeMirror typeMirror2 = type2.underlyingType; - QualifierHierarchy qh = type1.atypeFactory.getQualifierHierarchy(); - return qh.isSubtypeShallow(anno1, typeMirror1, anno2, typeMirror2) - && qh.isSubtypeShallow(anno2, typeMirror2, anno1, typeMirror1); - } else { + if (currentTop == null) { throw new BugInCF("currentTop null"); } + return arePrimaryAnnosEqual( + type1.getAnnotationInHierarchy(currentTop), + type2.getAnnotationInHierarchy(currentTop), + type1, + type2); + } + + /** + * Returns true if {@code anno1} (on {@code type1}) and {@code anno2} (on {@code type2}) are + * equal, i.e. each is a shallow subtype of the other. This overload takes the annotations to + * compare explicitly rather than reading them from the types, so a subclass can compare + * normalized/canonicalized annotations without mutating the types (see {@code + * ValueAnnotatedTypeFactory}); a mutating comparator cannot be used on the shared, frozen types + * returned by the annotated-type caches. + * + * @param anno1 the annotation on {@code type1} to compare, or null + * @param anno2 the annotation on {@code type2} to compare, or null + * @param type1 the first type (used for its underlying type and factory) + * @param type2 the second type + * @return true if {@code anno1} and {@code anno2} are equal + */ + protected boolean arePrimaryAnnosEqual( + AnnotationMirror anno1, + AnnotationMirror anno2, + AnnotatedTypeMirror type1, + AnnotatedTypeMirror type2) { + TypeMirror typeMirror1 = type1.underlyingType; + TypeMirror typeMirror2 = type2.underlyingType; + QualifierHierarchy qh = type1.atypeFactory.getQualifierHierarchy(); + return qh.isSubtypeShallow(anno1, typeMirror1, anno2, typeMirror2) + && qh.isSubtypeShallow(anno2, typeMirror2, anno1, typeMirror1); } /**