Cache a frequent conversion in the Value Checker

[wmdietl]

Maybe we should limit the size of the cache?

[wmdietl]

@copilot Can you measure whether this has any performance benefit compared to CI running on master?

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR introduces caching and cheaper equality checks to reduce overhead from frequent annotation conversions in the Value Checker.

Changes:

• Replaces set-based annotation equality checks in ValueVisitor with hierarchy-annotation retrieval.
• Adds an identity-based cache to avoid repeated non-special-case conversions in ValueAnnotatedTypeFactory.
• Refines widening logic to explicitly select the value-hierarchy annotation (and error if absent).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
framework/src/main/java/org/checkerframework/common/value/ValueVisitor.java	Avoids building AnnotationMirrorSet for integral cast safety checks by using hierarchy lookups.
framework/src/main/java/org/checkerframework/common/value/ValueAnnotatedTypeFactory.java	Adds an IdentityHashMap cache and adjusts conversion/widening logic to reduce repeated work.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

castAnno == exprAnno checks reference identity, not semantic equality. This is likely a behavioral regression from the prior AnnotationMirrorSet.equals(...) check: two equivalent AnnotationMirrors may be different instances, causing isTypeCastSafe to incorrectly return false and potentially emit spurious warnings. Use a semantic comparison (e.g., AnnotationUtils.areSame(castAnno, exprAnno) or an equivalent factory/hierarchy equality check) to preserve the prior behavior.

Suggested change

	if (castAnno == exprAnno) {
	if (AnnotationUtils.areSame(castAnno, exprAnno)) {

[Copilot]

This cache is unbounded and holds strong references to AnnotationMirror keys, which can grow without limit during long compilations or across many compilation units and increase memory retention. Consider bounding the cache (e.g., size-limited LRU) and/or clearing it at a known lifecycle boundary for the factory to avoid memory growth; alternatively, redesign the cache key to avoid storing per-instance AnnotationMirror objects when only a few “special” cases exist.

[Copilot]

Storing isSpecial=true entries doesn’t currently create a faster path (the method still computes annotationName(...) and re-checks all branches on every call), but it does increase cache size and memory retention. Either (a) only cache the non-special case (store false entries only), or (b) cache richer information (e.g., which special kind it is) so subsequent calls can skip annotationName(...) and branching while still handling differing max values.

[Copilot]

Storing isSpecial=true entries doesn’t currently create a faster path (the method still computes annotationName(...) and re-checks all branches on every call), but it does increase cache size and memory retention. Either (a) only cache the non-special case (store false entries only), or (b) cache richer information (e.g., which special kind it is) so subsequent calls can skip annotationName(...) and branching while still handling differing max values.

Suggested change

	if (cached == null) {
	specialIntRangeCache.put(anm, isSpecial);
	if (cached == null && !isSpecial) {
	specialIntRangeCache.put(anm, false);

[Copilot]

@copilot Can you measure whether this has any performance benefit compared to CI running on master?

I checked the CI data for this PR commit (006a0da) and master.

Current status: the PR CI run is still queued (CI tests run 24699635200), so there is no completed runtime yet to compare.

Master baseline from recent completed CI tests runs:

• last 10 runs: median 72.8 min, average 95.3 min (range 64.3–175.3)
• base SHA 70671f2 specifically: 72.1 min (schedule run 24657869149) and 129.6 min (push run 24644042584)

So I can’t yet claim a measured performance benefit from CI timing until run 24699635200 completes.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

• central.sonatype.com
  • Triggering command: /usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED --add-exports=jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED --add-opens=java.base/java.nio.charset=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.xml/javax.xml.namespace=ALL-UNNAMED -Xmx2g -Dfile.encoding=UTF-8 -Duser.country -Duser.language=en -Duser.variant -cp /home/REDACTED/.gradle/wrapper/dists/gradle-8.14.4-bin/92wwslzcyst3phie3o264zltu/gradle-8.14.4/lib/gradle-daemon-main-8.14.4.jar (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

• Configure Actions setup steps to set up my environment, which run before the firewall is enabled
• Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)