Skip to content

Improve performance for synthesizing a single annotation instance #24981

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sbrannen opened this issue Apr 27, 2020 · 1 comment
Closed

Improve performance for synthesizing a single annotation instance #24981

sbrannen opened this issue Apr 27, 2020 · 1 comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: declined A suggestion or change that we don't feel we should currently apply type: regression A bug that is also a regression

Comments

@sbrannen
Copy link
Member

Based on the findings in #24961 and #24970, it appears that we can improve the runtime performance for method parameter annotations synthesized due to their use of @AliasFor.

Annotations such as @RequestParam can only be declared directly on parameters (due to the @Target(ElementType.PARAMETER) declaration). Consequently, they can never be merged in the sense of findMergedAnnotation(). Similarly, the internal data structures in the MergedAnnotation API that support annotation attribute overriding (i.e., "merging") are never used for such annotations.

In addition, within the Spring Framework, @RequestParam is only looked up via SynthesizingMethodParameter, which delegates to AnnotationUtils.synthesizeAnnotation() to support the use of @AliasFor.

In 5.2.x, this is implemented using MergedAnnotation.from(...):

public static <A extends Annotation> A synthesizeAnnotation(
A annotation, @Nullable AnnotatedElement annotatedElement) {
if (annotation instanceof SynthesizedAnnotation || AnnotationFilter.PLAIN.matches(annotation)) {
return annotation;
}
return MergedAnnotation.from(annotatedElement, annotation).synthesize();
}

Whereas, in 5.1.x, this is implemented using the DefaultAnnotationAttributeExtractor:

static <A extends Annotation> A synthesizeAnnotation(A annotation, @Nullable Object annotatedElement) {
if (annotation instanceof SynthesizedAnnotation || hasPlainJavaAnnotationsOnly(annotatedElement)) {
return annotation;
}
Class<? extends Annotation> annotationType = annotation.annotationType();
if (!isSynthesizable(annotationType)) {
return annotation;
}
DefaultAnnotationAttributeExtractor attributeExtractor =
new DefaultAnnotationAttributeExtractor(annotation, annotatedElement);
InvocationHandler handler = new SynthesizedAnnotationInvocationHandler(attributeExtractor);
// Can always expose Spring's SynthesizedAnnotation marker since we explicitly check for a
// synthesizable annotation before (which needs to declare @AliasFor from the same package)
Class<?>[] exposedInterfaces = new Class<?>[] {annotationType, SynthesizedAnnotation.class};
return (A) Proxy.newProxyInstance(annotation.getClass().getClassLoader(), exposedInterfaces, handler);
}

The major difference here is that direct use of DefaultAnnotationAttributeExtractor avoids any extra overhead incurred by searching for annotation attribute overrides. In addition, DefaultAnnotationAttributeExtractor uses a direct approach for accessing the non-aliased values of the original annotation, thereby avoiding any complex lookups, conversions, adaptations, etc.

For example, if we still had a direct lookup mechanism for the required attribute in @RequestParam, then #24961 never would have surfaced as a regression since TypeMappedAnnotation.getValue(...) would not come into play.

In summary, the use of the MergedAnnotation API for direct synthesis of a single annotation without any form of attribute override support results in unnecessary runtime overhead and a regression in performance in Spring Framework 5.2.x.

We should therefore consider reintroducing the DefaultAnnotationAttributeExtractor / MapAnnotationAttributeExtractor dichotomy that existed prior to 5.2, or we should investigate an alternative means for synthesizing an annotation directly from a concrete annotation instance with limited support for @AliasFor for locally aliased attributes as is needed by clients such as SynthesizingMethodParameter.

@sbrannen sbrannen added for: team-attention in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression labels Apr 27, 2020
@sbrannen sbrannen added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 24, 2021
@sbrannen sbrannen added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 21, 2023
@sbrannen
Copy link
Member Author

Closing this issue since there have been no reports from the community regarding regressions in performance.

We may revisit this at a later date if we receive reports of performance regressions.

@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: declined A suggestion or change that we don't feel we should currently apply type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

1 participant