Avoid allocations for cached annotation mappings - #37141
Conversation
Defer creation of the visited annotation types set until a cache miss occurs. This avoids allocating a HashSet for every cached annotation mapping lookup while preserving recursive annotation handling during mapping creation. Signed-off-by: GT <gregjotau@gmail.com>
There was a problem hiding this comment.
Thanks for the PR and for the detailed benchmarking — that's great supporting evidence. 👍
You've introduced a new code path (Cache.get(Class)) for the cache-hit case, but no test exercises it directly. Right now it's only reached incidentally by tests targeting other behavior, so a regression in the cache-hit branch (e.g., returning a stale or incorrect mapping, or accidentally falling through to getOrCreate() every time) wouldn't necessarily be caught.
In light of that, please add a test in AnnotationTypeMappingsTests that calls AnnotationTypeMappings.forAnnotationType(...) twice for the same annotation type and asserts the second call returns the same cached instance (AssertJ: isSameAs()). That will pin down the cache-hit path this PR introduces.
Also, please introduce the static helper method I mentioned in the comments.
Cheers
| if (repeatableContainers == RepeatableContainers.standardRepeatables()) { | ||
| return standardRepeatablesCache.computeIfAbsent(annotationFilter, | ||
| key -> new Cache(repeatableContainers, key)).get(annotationType); | ||
| } | ||
| if (repeatableContainers == RepeatableContainers.none()) { | ||
| return noRepeatablesCache.computeIfAbsent(annotationFilter, | ||
| key -> new Cache(repeatableContainers, key)).get(annotationType); | ||
| } |
There was a problem hiding this comment.
This cache selection logic is now duplicated between this method and the 4-arg forAnnotationType(...) below, with an identical standardRepeatables()/none() dispatch that differs only in whether Cache.get(annotationType) or Cache.get(annotationType, visitedAnnotationTypes) gets invoked.
Let's extract a shared helper instead, perhaps something similar to the following.
private static @Nullable Cache getCache(
RepeatableContainers repeatableContainers, AnnotationFilter annotationFilter) {
if (repeatableContainers == RepeatableContainers.standardRepeatables()) {
return standardRepeatablesCache.computeIfAbsent(annotationFilter,
key -> new Cache(repeatableContainers, key));
}
if (repeatableContainers == RepeatableContainers.none()) {
return noRepeatablesCache.computeIfAbsent(annotationFilter,
key -> new Cache(repeatableContainers, key));
}
return null;
}Both overloads would then reduce to a null-check on getCache(...), which would avoid having two copies of the dispatch logic to keep in sync as this evolves.
There was a problem hiding this comment.
Done — both overloads now go through a shared getCache(...) helper as suggested.
Extract a shared getCache helper to avoid duplicating cache selection logic, and add a test that subsequent lookups return the same cached instance. Signed-off-by: GT <gregjotau@gmail.com>
|
Thanks for the review. Addressed in f0a932b:
|
AnnotationTypeMappings.forAnnotationType(...)currently creates a newHashSetfor visited annotation types before checking whether the mappings arealready cached. Cached lookups therefore allocate a set that is never used.
This change adds a cache-hit path that defers creation of the visited set until
a cache miss. Mapping creation and recursive annotation handling continue to
use the same visited-set logic.
Benchmarks
I measured 20 million cached mapping lookups after 2 million warmup lookups on
JDK 26.0.2 with compact object headers and
-XX:TieredStopAtLevel=1. Limitingtiered compilation models the cold/C1-compiled startup path where JFR showed
the allocation; fully warmed C2 can scalar-replace it.
I also patched only these two generated class files into Spring Core 7.0.8 and
recorded startup of a large Kotlin/Spring Boot application with 409 dependency
jars and 191 Spring Data repositories. JFR allocation samples changed as
follows:
HashSetfromAnnotationTypeMappingsWall-clock application startup was too noisy to make an end-to-end timing
claim, so the timing result above is limited to the isolated cached lookup.
Verification
./gradlew :spring-core:checkpasses, including the JDK 21 and JDK 24 testsuites, multi-release JAR validation, architecture checks, and checkstyle.