Immutable AnnotatedTypeMirror with freeze#1798
Merged
Merged
Conversation
AnnotatedTypeMirrors are mutable, so every cache in AnnotatedTypeFactory defensively deep-copies on both store and return. AnnotatedTypeCopier.visit is ~2% self-time and the dominant share of Object[] TLAB allocation on a realistic checkNullness compile, and the deep-copy tax is what makes the cached types cost +50-70 MB of retained heap (see docs/developer/performance-notes.md, the value-semantics program). The goal is to share frozen cached instances instead of copying them. This adds the freeze mechanism without using it yet, so it is behavior-neutral: - A private `frozen` bit plus `isFrozen()`. - `checkMutable()`, called by the three primary-annotation sinks (addAnnotation, removeAnnotation, clearAnnotations), throws BugInCF when the type is frozen. Every other annotation mutator routes through these. freeze() also calls primaryAnnotations.makeUnmodifiable() as a backstop for the getAnnotationsField() and AnnotatedDeclaredTypeNoHierarchy.addAnnotation paths. - A cycle-safe deep freeze() that freezes only already-initialized structural components (the frozen bit doubles as the visited marker). Lazy getters freeze any component they create while the owner is frozen, so the reachable graph stays frozen without eagerly materializing bounds and type arguments. Structural setters are intentionally left unguarded: the corruption vector is annotation mutation, and deep freeze() already freezes every reachable component's annotations. Nothing calls freeze() yet; freezing cache masters and sharing them is the subject of follow-up changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…che masters The annotated-type caches in AnnotatedTypeFactory deep-copy a value on store so the stored "master" is never aliased. Freezing those masters (rejecting later in-place mutation) is the first enforceable step of the value-semantics program; it turns latent cache-corruption bugs into immediate BugInCF instead of silent wrong results. Freezing flushed a single root-cause bug: AnnotatedTypeCopier.visitExecutable set the copy's vararg type to the original's vararg type by reference rather than copying it, so deepCopy() of an executable type was not fully independent -- the copy shared the original's vararg array subtree (Object[], Class<?>[], LinkOption[], ...). Defaulting then mutated that shared subtree. It was harmless before only because defaulting adds the same deterministic annotations either way. The copier now copies the vararg type through the visitor (originalToCopy maps it to the already-made parameter copy when the vararg is the last parameter, else it is freshly copied). With that fix, freeze the master at all eight cache store sites via a new frozenDeepCopy helper (elementCache, elementTypeCache, classAndMethodTreeCache, fromMemberTreeCache, fromExpressionTreeCache, fromTypeTreeCache, methodAsMemberOfCache) and on the directSupertypesCache list. The caches still return a fresh deepCopy() on every hit, so this is behavior-neutral; it only adds the fail-fast invariant. Removing the copy-on-return (the allocation win) is the subject of follow-up changes. Passes the full framework, javacutil, dataflow, and checker JUnit suites (125 test suites, 0 failures), including AllSystemsTest. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
VarargCacheAliasing calls JDK vararg methods (Arrays.asList, String.format, Class.getMethod) whose executable types are cached and re-defaulted. With the cache masters frozen and the AnnotatedTypeCopier vararg fix reverted, this crashes with "Attempted to mutate a frozen AnnotatedTypeMirror", because deepCopy() shared the original's vararg subtree instead of copying it. The test type-checks cleanly with the fix and guards against a regression of it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Update performance-notes.md with the PR #1798 results and the session's findings: - Applied: new "AnnotatedTypeMirror immutability foundation" entry for the freeze() mechanism, the AnnotatedTypeCopier vararg-aliasing fix, and frozen cache masters (behavior- and perf-neutral; measured allocation +/-0.1%, freeze() below the on-CPU sampling threshold). - Tried and rejected: the cache-boundary flips (the cache-return deepCopy is load-bearing because defaulting, flow refinement, type-argument inference, and constructorFromUse all mutate the returned type), hashCode caching on frozen types (0% of hashCode calls hit frozen types), and the shallow-location defaulting shortcut (10.2% fewer scans but over cheap types, allocation flat). - Value-semantics narrative: the "one copier bug, not pervasive aliasing" finding, the benign-to-results finding (the bug only surfaces as the freeze crash, so the regression test needs the freeze enforcement), and the load-bearing-copy conclusion that the allocation win needs copy-on-write or eliminating redundant re-annotation, not a boundary flip. - Status: the immutability program is paused at its shipped foundation; it is no longer the "recommended next direction" until a copy-on-write prototype is measured. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The freeze() additions sit immediately before AnnotatedTypeVariable.getBounds() and AnnotatedWildcardType.fixupBoundAnnotations(), so the diff-based javadoc doclint check attributes those methods' pre-existing "no comment" warnings to this PR. Add Javadoc to getBounds(), the adjacent getBoundFields(), and the wildcard fixupBoundAnnotations(). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
StructuralEqualityComparer.arePrimaryAnnosEqual compared the primary annotations of two types. ValueAnnotatedTypeFactory overrode it to first normalize the operands' annotations to a canonical form (e.g. so @IntRange(1,1) and @intval(1) compare equal) -- but it did so by calling replaceAnnotation on type1 and type2, i.e. an equality check with a side effect on its operands. That is incorrect in principle and prevents a type comparison from being run on a shared, immutable (frozen) type, which the annotated-type caches will hand out. Add a non-mutating overload arePrimaryAnnosEqual(anno1, anno2, type1, type2) that takes the annotations to compare explicitly, and refactor the base method to use it. The Value override now computes the canonical annotations and compares them via the overload, without mutating the types. Behavior-preserving: the normalized annotations were only used for the comparison, not relied upon afterward. Passes framework, all Value, and all-systems test suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…f copying getAnnotatedType(Tree) for a class or method declaration deep-copied the cached type on every hit so callers could mutate it. Most callers only read it, so the copy was wasted. Because the cached masters are frozen (immutable), the hit path now returns the shared frozen value and the callers that actually mutate it copy it first: - getMethodReturnType copies the return type when it is frozen (its overrides mutate it). - InitializationParentAnnotatedTypeFactory.getSelfType copies before mutating the self type and its enclosing types. - getAnnotatedType(Tree) copies the from* result if frozen before addComputedTypeAnnotations (an expression's type can be a class/method type served from the cache). - constructorFromUse copies the frozen enclosing type before embedding and annotating it. - ValueVisitor.checkOverride copies the executable types before replaceSpecialIntRangeAnnotations mutates them. Deterministic-allocation A/B (single forked javac): about -1% on a method-heavy single-class file and within noise on a realistic file -- classAndMethodTreeCache is a low-volume cache, so this is GC-pressure relief, not a wall-clock win, and it establishes the copy-on-frozen pattern for sharing cached types. Passes the full framework and checker JUnit suites (125 test suites, 0 failures). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
) Update the value-semantics narrative and open-venues with the PR #1798 results: the cross-cutting side-effecting-equality blocker (now fixed non-mutating), the first shipped boundary flip (classAndMethodTreeCache, green but ~-1%/~0% because it is low-volume), and the finding that the high-volume elementTypeCache is mutation-dominated (asMemberOf) so its flip win is also likely modest. The flip is mechanically unblockable via copy-on-frozen at the enumerated mutating consumers; the larger win still needs copy-on-write or eliminating redundant re-annotation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getAnnotatedType(Element) deep-copied the cached type on every hit so callers could mutate it freely. With the cache master frozen (the PR #1798 tripwire), return the shared frozen instance directly and copy-on-frozen only at the callers that actually mutate the result: - computeMethodTypeAsMemberOf (the unsubstituted method type) - constructorFromUse (the diamond type, the constructor, and the anonymous-class super constructor) - AnnotatedTypes.asMemberOf (the member type it may alias and postAsMemberOf mutates) - SyntheticArrays.replaceReturnType (array clone: mutates then the caller mutates again) - DependentTypesHelper.atInvocation and KeyForDependentTypesHelper .atMethodInvocation (re-fetch the declared method type and viewpoint-adapt it in place) - AnnotationFileParser.processFakeOverride (the stub parser side-effects a type it documents as "fresh") This removes a deep copy per cache hit for the read-only majority of callers. elementTypeCache is the largest annotated-type cache, but a full-checknullness re-trace shows AnnotatedTypeCopier is now only ~0.76% self-time / ~1.5% of allocation (earlier caches already harvested most of it), so the win is modest: deterministic ThreadAllocationStatistics drops ~1% on generic-call-heavy code (Big300 -0.75%, Big600 -0.97%) and is within noise elsewhere. Shipped for the read-only-copy elimination and the enforced no-mutate-cached-types invariant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…#1798) Document the immutability program's measured outcome honestly: - the elementTypeCache and classAndMethodTreeCache flips pay ~1% on generic-call-heavy code and noise elsewhere -- not the -5.3% an earlier estimate suggested, which never reproduced against the post-flip baseline; - the directSupertypesCache flip was tried and rejected (its dominant consumer, AsSuperVisitor, mutates each returned supertype in place, so the flip relocates the per-hit copy rather than removing it -- the same trap as the four pre-pipeline caches and methodAsMemberOf); - a post-mortem on why the win was modest: a fresh full-checknullness trace shows AnnotatedTypeCopier is now ~0.76% self-time / ~1.5% of allocation, far below the ~22%-of-Object[] figure that motivated the program -- intervening caches (PR #1777), the thread-local copier map, and lazy visitedNodes had already harvested it. Lesson: re-trace the current baseline before committing to a plan built on an older profile. Also corrects the stale -1.9%/-5.3% figures previously written into this file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The flip (commit "share frozen elementTypeCache values instead of copying") returned the shared frozen cache master from getAnnotatedType(Element). A full Guava nullness build -- which alltests does not cover -- crashed with "Attempted to mutate a frozen AnnotatedTypeMirror with underlying type java.lang.Object". Root cause: a consumer reparents 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<?, K> with K extends Comparable) -- into a fresh, non-frozen result type; addComputedTypeAnnotations then mutates the frozen child. The copy-on-frozen guards added with the flip all check the root (if (type.isFrozen()) deepCopy()), so a non-frozen root holding a frozen child slips through. This hazard is structural to returning a shared frozen value -- any path that lifts a child out of the shared master is a latent crash -- and is not worth the flip's ~1% allocation win. This reverts the flip and its nine copy-on-frozen consumer sites. A frozen-master tripwire that still returns deepCopy() remains safe (the master is never handed out), so the frozen cache masters stay. The lower-traffic classAndMethodTreeCache flip is kept (it survived the Guava build and alltests). Adds a regression test minimized from com.google.common.collect.SortedLists.binarySearch; performance-notes records the embedded-frozen-component hazard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The flip writeup documented the embedded-frozen-component hazard and the revert, but not how (or whether) the flip could be salvaged. Add the cost ladder: (1) fix each reparenting site -- cheap but unenumerable, a convention not enforced; (2) deep guard at the choke point -- complete but the scan likely erases the ~1% win; (3) copy-on-write ATMs -- the only complete fix, which would make all eight caches flippable, but is a separate measured project, not a patch. Verdict: keep reverted; pursue via copy-on-write if at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The original plan was larger, but Claude ran into blocking issues.
This is a memory and performance neutral change that allows us to ensure some modifications are avoided.
Originally two caches were serving frozen objects, but then a Guava failure undid one of these changes.
So not a huge impact, but hopefully it will allow us to explore immutability more.