Skip to content
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

Avoid unnecessary synthesizable annotation processing [SPR-16933] #21472

Closed
spring-issuemaster opened this issue Jun 12, 2018 · 10 comments
Closed

Avoid unnecessary synthesizable annotation processing [SPR-16933] #21472

spring-issuemaster opened this issue Jun 12, 2018 · 10 comments
Assignees
Milestone

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Jun 12, 2018

Phil Webb opened SPR-16933 and commented

I've been looking at the performance involved with using the SimpleMetadataReader and I suspect that it might be possible to improve AnnotationUtils.

I started with the following application that simulates the class parsing involved in a typical Spring Boot application

https://gist.github.com/philwebb/a22ea7bf5b575abd058a748fe12838d0

This app takes about 130ms to read the metadata from those 106 classes.
If I change the code to use an empty ASM visitor, parsing takes ~30ms
Reading just class bytes into an array takes ~22ms (so ASM parsing itself is quite fast)

If I remove the code that visits annotations so no AnnotationAttributes are read, it takes ~60ms.

Looking at the profiler a lot of time is spent in AnnotationUtils so I created a second sample:

https://gist.github.com/philwebb/f7cdc99401af8e063853b2ed574e9277

The NAMES being used here are the same as were ultimately used with the first sample. This one is reading annotation data for 413 classes (starting from their String name) and it takes ~93ms.
 


Affects: 5.0.7

Attachments:

Issue Links:

  • #20609 Annotations on generic interface methods not found by AnnotationUtils
  • #21208 Reduce ClassUtils.forName overhead (in particular for annotation introspection purposes)
  • #21216 Comprehensively cache annotated methods for interfaces and superclasses
  • #21271 Avoid repeated superclass introspection in AnnotationUtils.findAnnotation
  • #19660 Spring internal configuration classes can no longer use @EventListener
  • #21702 @Scheduled task runs twice on bean with target-class scoped proxy (when injected)

0 votes, 6 watchers

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 12, 2018

Phil Webb commented

Looking at the profiler, AnnotationUtils.isSynthesizable might be a hot spot. Also, the fact that a proxy can be created might also be problematic.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jun 12, 2018

Phil Webb commented

Updating the sample to just get immediate annotations and only once per class gives 32ms

public static void main(String[] args) throws ClassNotFoundException {
	LogFactory.getLog(TimeMe2.class);
	Set<String> names = new HashSet<>(Arrays.asList(NAMES));
	long t = System.nanoTime();
	for (String name : names) {
		Class.forName(name).getAnnotations();
	}
	System.err.println(TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - t));
}
@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 31, 2018

Juergen Hoeller commented

Phil Webb, since there seem to be some immediate optimizations that we can apply to AnnotationUtils.isSynthesizable, even backportable to 5.0.9, I'll narrow the scope of this ticket to that specific hotspot. Let's deal with a wider-ranging refactoring in a separate ticket.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jul 31, 2018

Juergen Hoeller commented

I've done a revision on master which considers any java/javax and org.springframework.lang annotations as not synthesizable upfront, checking not only in isSynthesizable but also at synthesizeAnnotation(Array) level and therefore bypassing such overhead (even the reflective Annotation.annotationType() call) for these common meta-annotations. This shows for classpath scanning in particular since it optimizes the common AnnotationUtils code paths entered by ASM; additionally, we're also skipping meta-annotation introspection for java.lang.annotation types in AnnotationAttributesReadingVisitor upfront.

I'll backport a variant of this to 5.0.9, being less aggressive with exclusion and primarily focused on avoiding java.lang.annotation introspection overhead (which covers a lot of cases already).

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 1, 2018

Juergen Hoeller commented

Slightly extended scope again: We're avoiding synthesizable annotation creation for @Bean / @Scope processing now, interacting with AnnotationAttributes in BeanAnnotationHelper and streamlining towards plain annotation introspection in AnnotationAttributesReadingVisitor since we only need attributes there anyway.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 7, 2018

Sam Brannen commented

Thanks for tackling all of this, Juergen Hoeller!

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 15, 2018

Juergen Hoeller commented

For 5.1, we also cache declared annotation arrays now, avoiding GC pressure for the reflection-returned array clones.

Last but not least, AnnotatedElementUtils.get/findAllMergedAnnotations has variants with a set of annotation types to look for, so that SpringCacheAnnotationParser can perform combined searching for all of its cache operation annotations in one step. This is also 5.1 only.

@spring-issuemaster
Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Jan 4, 2019

fbutter commented

Juergen Hoeller, in this commit a condition was added to exclude beans without a Component annotation. Consequently beans provided by bean methods are not considered as listener anymore. This is a major change and e.g. spring cloud is not compatible anymore (spring-cloud-consul-discovery is not compatible with Spring Boot 2.1.1.RELEASE #481|https://github.com/spring-cloud/spring-cloud-consul/issues/481]). I could not find this in the release notes, was this intended?

@jhoeller
Copy link
Contributor

@jhoeller jhoeller commented Jan 31, 2019

For the record, the @EventListener revision was an intentional optimization that only affected org.springframework classes (i.e. not user-provided application components) and was communicated across the Spring portfolio projects accordingly. If any issues remain here, they should be fixed in the affected portfolio projected through annotating the components accordingly, or preferably through implementing ApplicationListener instead (as the preferred option for infrastructure components).

@sbrannen
Copy link
Member

@sbrannen sbrannen commented Mar 3, 2019

I just got bitten by this change... in an event listener in our own test suite, where the package is org.springframework.test.context.event. Thus, the isSpringContainerClass() obviously returns true for such types, and it took me a bit of debugging to figure it out. 😱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.