From 8265db0ae1d83a1e204e602fa96e542c2677977f Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Thu, 23 Apr 2020 18:51:45 +0200 Subject: [PATCH] Avoid synthesizing annotations unnecessarily Commit 31fa1569c5 introduced initial support for avoiding unnecessary annotation synthesis in the MergedAnnotation API; however, it only avoided synthesis for annotations that do not declare any attributes. This commit reworks this support to avoid unnecessary annotation synthesis for annotations that declare attributes. Specifically, this commit introduces a new `isSynthesizable()` method in AnnotationTypeMapping that allows the "synthesizable" flag to be computed once and cached along with the other metadata already cached in AnnotationTypeMapping instances. TypeMappedAnnotation now delegates to this new method when determining whether it should synthesize an annotation. Closes gh-24861 --- .../annotation/AnnotationTypeMapping.java | 55 +++++++++++++++++++ .../core/annotation/MergedAnnotation.java | 24 ++++++-- .../core/annotation/TypeMappedAnnotation.java | 19 ++++--- .../annotation/MergedAnnotationsTests.java | 23 +++++++- 4 files changed, 104 insertions(+), 17 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMapping.java b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMapping.java index 657e34d5a5..ef87e2d066 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMapping.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/AnnotationTypeMapping.java @@ -77,6 +77,8 @@ final class AnnotationTypeMapping { private final Map> aliasedBy; + private final boolean synthesizable; + private final Set claimedAliases = new HashSet<>(); @@ -101,6 +103,7 @@ final class AnnotationTypeMapping { processAliases(); addConventionMappings(); addConventionAnnotationValues(); + this.synthesizable = computeSynthesizableFlag(); } @@ -307,6 +310,47 @@ final class AnnotationTypeMapping { return !isValueAttribute && existingDistance > mapping.distance; } + @SuppressWarnings("unchecked") + private boolean computeSynthesizableFlag() { + // Uses @AliasFor for local aliases? + for (int index : this.aliasMappings) { + if (index != -1) { + return true; + } + } + + // Uses @AliasFor for attribute overrides in meta-annotations? + if (!this.aliasedBy.isEmpty()) { + return true; + } + + // Uses convention-based attribute overrides in meta-annotations? + for (int index : this.conventionMappings) { + if (index != -1) { + return true; + } + } + + // Has nested annotations or arrays of annotations that are synthesizable? + if (getAttributes().hasNestedAnnotation()) { + AttributeMethods attributeMethods = getAttributes(); + for (int i = 0; i < attributeMethods.size(); i++) { + Method method = attributeMethods.get(i); + Class type = method.getReturnType(); + if (type.isAnnotation() || (type.isArray() && type.getComponentType().isAnnotation())) { + Class annotationType = + (Class) (type.isAnnotation() ? type : type.getComponentType()); + AnnotationTypeMapping mapping = AnnotationTypeMappings.forAnnotationType(annotationType).get(0); + if (mapping.isSynthesizable()) { + return true; + } + } + } + } + + return false; + } + /** * Method called after all mappings have been set. At this point no further * lookups from child mappings will occur. @@ -478,6 +522,17 @@ final class AnnotationTypeMapping { return this.mirrorSets; } + /** + * Determine if the mapped annotation is synthesizable. + *

Consult the documentation for {@link MergedAnnotation#synthesize()} + * for an explanation of what is considered synthesizable. + * @return {@code true} if the mapped annotation is synthesizable + * @since 5.2.6 + */ + boolean isSynthesizable() { + return this.synthesizable; + } + private static int[] filledIntArray(int size) { int[] array = new int[size]; diff --git a/spring-core/src/main/java/org/springframework/core/annotation/MergedAnnotation.java b/spring-core/src/main/java/org/springframework/core/annotation/MergedAnnotation.java index cba6ae1880..ed60e365c3 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/MergedAnnotation.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/MergedAnnotation.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -479,11 +479,23 @@ public interface MergedAnnotation { > T asMap(Function, T> factory, Adapt... adaptations); /** - * Create a type-safe synthesized version of this annotation that can be - * used directly in code. + * Create a type-safe synthesized version of this merged annotation that can + * be used directly in code. *

The result is synthesized using a JDK {@link Proxy} and as a result may * incur a computational cost when first invoked. - * @return a synthesized version of the annotation. + *

If this merged annotation was created {@linkplain #from(Annotation) from} + * an annotation instance, that annotation will be returned unmodified if it is + * not synthesizable. An annotation is considered synthesizable if + * one of the following is true. + *

+ * @return a synthesized version of the annotation or the original annotation + * unmodified * @throws NoSuchElementException on a missing annotation */ A synthesize() throws NoSuchElementException; @@ -493,8 +505,10 @@ public interface MergedAnnotation { * on a condition predicate. *

The result is synthesized using a JDK {@link Proxy} and as a result may * incur a computational cost when first invoked. + *

Consult the documentation for {@link #synthesize()} for an explanation + * of what is considered synthesizable. * @param condition the test to determine if the annotation can be synthesized - * @return a optional containing the synthesized version of the annotation or + * @return an optional containing the synthesized version of the annotation or * an empty optional if the condition doesn't match * @throws NoSuchElementException on a missing annotation * @see MergedAnnotationPredicates diff --git a/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java b/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java index 15c674f9e3..1704045400 100644 --- a/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java +++ b/spring-core/src/main/java/org/springframework/core/annotation/TypeMappedAnnotation.java @@ -331,19 +331,20 @@ final class TypeMappedAnnotation extends AbstractMergedAnn @Override @SuppressWarnings("unchecked") protected A createSynthesized() { - if (this.rootAttributes instanceof Annotation) { - // Nothing to synthesize? - if (this.resolvedRootMirrors.length == 0 && this.resolvedMirrors.length == 0) { - return (A) this.rootAttributes; - } - // Already synthesized? - if (this.rootAttributes instanceof SynthesizedAnnotation) { - return (A) this.rootAttributes; - } + if (getType().isInstance(this.rootAttributes) && !isSynthesizable()) { + return (A) this.rootAttributes; } return SynthesizedMergedAnnotationInvocationHandler.createProxy(this, getType()); } + private boolean isSynthesizable() { + // Already synthesized? + if (this.rootAttributes instanceof SynthesizedAnnotation) { + return false; + } + return this.mapping.isSynthesizable(); + } + @Override public String toString() { String string = this.string; diff --git a/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java b/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java index 9f113fc26b..2f1d23e111 100644 --- a/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java +++ b/spring-core/src/test/java/org/springframework/core/annotation/MergedAnnotationsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -1380,14 +1380,22 @@ class MergedAnnotationsTests { @Test void synthesizeShouldNotSynthesizeNonsynthesizableAnnotations() throws Exception { Method method = getClass().getDeclaredMethod("getId"); + Id id = method.getAnnotation(Id.class); assertThat(id).isNotNull(); - Id synthesizedId = MergedAnnotation.from(id).synthesize(); assertThat(id).isEqualTo(synthesizedId); - // It doesn't make sense to synthesize {@code @Id} since it declares zero attributes. + // It doesn't make sense to synthesize @Id since it declares zero attributes. assertThat(synthesizedId).isNotInstanceOf(SynthesizedAnnotation.class); assertThat(id).isSameAs(synthesizedId); + + GeneratedValue generatedValue = method.getAnnotation(GeneratedValue.class); + assertThat(generatedValue).isNotNull(); + GeneratedValue synthesizedGeneratedValue = MergedAnnotation.from(generatedValue).synthesize(); + assertThat(generatedValue).isEqualTo(synthesizedGeneratedValue); + // It doesn't make sense to synthesize @GeneratedValue since it declares zero attributes with aliases. + assertThat(synthesizedGeneratedValue).isNotInstanceOf(SynthesizedAnnotation.class); + assertThat(generatedValue).isSameAs(synthesizedGeneratedValue); } /** @@ -2987,7 +2995,16 @@ class MergedAnnotationsTests { @interface Id { } + /** + * Mimics javax.persistence.GeneratedValue + */ + @Retention(RUNTIME) + @interface GeneratedValue { + String strategy(); + } + @Id + @GeneratedValue(strategy = "AUTO") private Long getId() { return 42L; }