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

Reduce ClassUtils.forName overhead (in particular for annotation introspection purposes) [SPR-16667] #21208

Closed
spring-projects-issues opened this issue Mar 29, 2018 · 17 comments
Assignees
Labels
in: core status: backported type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Mar 29, 2018

Christoph Dreis opened SPR-16667 and commented

Hi,

while analyzing the calls to ClassUtils.forName() in my Spring-Boot app I noticed that there might be an opportunity to optimize some more calls of commonly known classes. E.g. when looking for calls to Java classes I saw the following distribution for a total of 13000 calls (roughly 3000 coming from Java classes):

Class Calls
java.lang.Cloneable 1168
java.io.Serializable 790
java.lang.String 473
java.lang.Enum 292
java.util.List 150
java.lang.Object 82
java.lang.Long 74
java.util.Map 52
java.util.Collection 52
java.lang.Class 41
java.util.Optional 39
java.lang.Integer 32
... ...

As you notice especially Serializable and Cloneable take up a big portion here, which are missing from the common class cache. Also Enum and the Collection framework interfaces contribute to the calls.

An isolated JMH benchmark for Serializable shows the following results:

Benchmark Mode Cnt Score Error Units
MyBenchmark.testNew thrpt 10 102632276,261 ± 37687656,076 ops/s
MyBenchmark.testNew: gc.alloc.rate thrpt 10 0,001 ± 0,001 MB/sec
MyBenchmark.testNew: gc.alloc.rate.norm thrpt 10 ? 10?? B/op
MyBenchmark.testOld thrpt 10 1072135,584 ± 38640,911 ops/s
MyBenchmark.testOld:·gc.alloc.rate thrpt 10 138,990 ± 5,010 MB/sec
MyBenchmark.testOld:·gc.alloc.rate.norm thrpt 10 136,001 ± 0,001 B/op

I understand that drawing the line is a difficult thing to do - in fact I had the same problem - but I'd argue we could add the Collection interfaces, Enum and Optional and the interfaces contained in the javaLanguageInterfaces field to the common class cache. That seems like a reasonable collection of commonly known classes.

I'd be happy to hear your opinion on the attached PR.

Cheers,
Christoph


Affects: 4.3.14, 5.0.4

Issue Links:

  • #18199 Performance regression on startup (in particular in AnnotationUtils)
  • #16341 Metadata reading should never use ASM for java.* and javax.* types (in particular on JDK 8)
  • #20869 CachedIntrospectionResults should use BeanInfoFactory when introspecting implemented interfaces
  • #21216 Comprehensively cache annotated methods for interfaces and superclasses
  • #21472 Avoid unnecessary synthesizable annotation processing

Referenced from: pull request #1761, and commits 4da27c2, 5d54adf, 22a8a66, 7a8d41e

Backported to: 4.3.15

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 29, 2018

Juergen Hoeller commented

As mentioned in my commit comment, I'd register Iterable and Iterator as well. Any further candidates that come to your mind in the immediate vicinity?

Feel free to extend your commit along those lines; I'll merge it right away then.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 29, 2018

Christoph Dreis commented

Thanks. Well, actually Duration shows up as well - which I had in my initial list - but decided not to include it as it brings in java.time. I could add this as well, though - I have no strong feelings against it. What do you think?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 29, 2018

Juergen Hoeller commented

I'd rather draw the line there and avoid any java.time imports at that level. java.lang + java.util + java.io seem to be just about right.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 29, 2018

Juergen Hoeller commented

Beyond such core classes by default, we could introduce a 'live' cache for other common classes where ClassUtils.forName caches any java. and javax. classes on demand. However, I wonder where so many forName calls come from in the first place... maybe such caching could also happen at the call sites?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Christoph Dreis commented

Sorry, I didn't manage to make the changes yesterday. but I see you closed the PR already. :( Could you point me to your commit? I can't seem to find it

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Juergen Hoeller commented

ClassUtils.isJavaLanguageInterface came from #20869 but also makes sense to be added to the common cache classes indeed. I've added Closeable and AutoCloseable there as well, along with the general additions to the common class cache.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Juergen Hoeller commented

Oh just noticed your latest comment: This is about to be committed along with a few other things. Please stay tuned for a further minute or two :-)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Juergen Hoeller commented

It's in master now, along with the extended Java language interface set.

BTW, any indication where all those forName calls are coming from? Is this all happening during early initialization where we're trying to avoid hard references to classes?

As for the idea of a live cache, we could pick that up separately if really needed. I have a suspicion that we might be better off fine-tuning the common callers though.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Juergen Hoeller commented

FYI, we have a hard release deadline on Tuesday, right after Easter. We're trying to wrap up all 5.0.5 work and do a final backport to 4.3.15 today. If there is anything else you'd like to raise, now is your chance for another few hours :-)

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Christoph Dreis commented

Lots of the calls for the interfaces seem to come from AnnotationTypeFilter or AbstractTypeHierarchyTraversingFilter (depending on what class you think should skip the interface).

	  at org.springframework.util.ClassUtils.forName(ClassUtils.java:216)
	  at org.springframework.core.type.filter.AnnotationTypeFilter.hasAnnotation(AnnotationTypeFilter.java:103)
	  at org.springframework.core.type.filter.AnnotationTypeFilter.matchInterface(AnnotationTypeFilter.java:94)
	  at org.springframework.core.type.filter.AbstractTypeHierarchyTraversingFilter.match(AbstractTypeHierarchyTraversingFilter.java:96)
	  at org.springframework.core.type.filter.AbstractTypeHierarchyTraversingFilter.match(AbstractTypeHierarchyTraversingFilter.java:121)
	  at org.springframework.core.type.filter.AbstractTypeHierarchyTraversingFilter.match(AbstractTypeHierarchyTraversingFilter.java:81)
	  at org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider.isCandidateComponent(ClassPathScanningCandidateComponentProvider.java:354)
	  at org.springframework.context.annotation.ClassPathScanningCandidateComponentProvider.findCandidateComponents(ClassPathScanningCandidateComponentProvider.java:288)
	  at org.springframework.data.repository.config.RepositoryComponentProvider.findCandidateComponents(RepositoryComponentProvider.java:122)
	  at org.springframework.data.repository.config.RepositoryConfigurationSourceSupport.getCandidates(RepositoryConfigurationSourceSupport.java:76)
	  at org.springframework.data.repository.config.RepositoryConfigurationExtensionSupport.getRepositoryConfigurations(RepositoryConfigurationExtensionSupport.java:83)
	  at org.springframework.data.repository.config.RepositoryConfigurationDelegate.registerRepositoriesIn(RepositoryConfigurationDelegate.java:123)
	  at org.springframework.boot.autoconfigure.data.AbstractRepositoryConfigurationSourceSupport.registerBeanDefinitions(AbstractRepositoryConfigurationSourceSupport.java:59)
	  at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.loadBeanDefinitionsFromRegistrars(ConfigurationClassBeanDefinitionReader.java:359)
	  at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.loadBeanDefinitionsForConfigurationClass(ConfigurationClassBeanDefinitionReader.java:143)
	  at org.springframework.context.annotation.ConfigurationClassBeanDefinitionReader.loadBeanDefinitions(ConfigurationClassBeanDefinitionReader.java:116)
	  at org.springframework.context.annotation.ConfigurationClassPostProcessor.processConfigBeanDefinitions(ConfigurationClassPostProcessor.java:320)
	  at org.springframework.context.annotation.ConfigurationClassPostProcessor.postProcessBeanDefinitionRegistry(ConfigurationClassPostProcessor.java:228)
	  at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanDefinitionRegistryPostProcessors(PostProcessorRegistrationDelegate.java:272)
	  at org.springframework.context.support.PostProcessorRegistrationDelegate.invokeBeanFactoryPostProcessors(PostProcessorRegistrationDelegate.java:92)
	  at org.springframework.context.support.AbstractApplicationContext.invokeBeanFactoryPostProcessors(AbstractApplicationContext.java:687)

So one could argue we can skip the call already there for the well-known (annotation-less) interfaces. In fact, AnnotationTypeFilter does so already for Object.class. We could extend this to make use of a common-class cache as well. What do you think?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Juergen Hoeller commented

Interesting, so this seems to be a consequence of #16341 where we're not reading such interfaces via ASM anymore... but put some load on ClassUtils.forName now. As with the original #16341, this is primarily a problem with Spring Data's repository interfaces. I'll see what I can do about this today still: maybe a simple condition that assumes that a java* interface won't ever have a non-java* annotation on it...

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Juergen Hoeller commented

That fine-tuned rule for standard Java types in AnnotationTypeFilter seems to make quite a difference, so let's give it a try in 5.0.5, also backporting it (and maybe a few - but not all - extra common classes in ClassUtils) to 4.3.15.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Christoph Dreis commented

I highly appreciate your efforts. Thanks. Enjoy your Easter days.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Juergen Hoeller commented

Thanks for pointing out those hotspots! Enjoy the extended weekend as well...

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 30, 2018

Juergen Hoeller commented

To wrap up this effort, I've added a similar standard Java type rule to AnnotatedElementUtils: not searching standard Java annotations for anything other than standard Java meta-annotations. And on a related note, the annotated interface cache in AnnotationUtils specifically ignores @Nullable for its purposes now.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 31, 2018

Christoph Dreis commented

I didn't expect that we end up optimizing my long time enemy "annotation processing" when I filed this ticket ;-). I wonder if we will hit the second condition currentAnnotationType.getName().startsWith("java") that often, though - given that we already check if the annotation is in the Java annotation package. IMHO, that should get most overhead out of the way.
Anyhow - thanks a lot, Jürgen.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Mar 31, 2018

Juergen Hoeller commented

As for the "java" check, this is meant to kick in for Java EE annotations and also for java.lang.FunctionalInterface etc where we never need to search for non-standard annotations on them. The existing java.lang.annotation check was only meant to cover the annotation meta-descriptors from java.lang.annotation which only ever appear as meta-annotations themselves.

In the meantime, I got the follow-up #21216 in the works, revising our annotatedInterfaceCache in AnnotationUtils to an annotatedBaseTypeCache (in particular for AnnotatedElementUtils find semantics), also considering superclasses... and storing the actual annotated Method handles now rather than just a flag that there is something to consider somewhere on that type. This brings dramatic performance benefits for certain annotation lookup scenarios... primarily motivated by recent Boot 2.0 performance benchmarks on our side but your ticket here touched upon that area, so I figured I'd give that caching revision a go for 5.0.5 as well.

So much for wrapping up yesterday ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core status: backported type: enhancement
Projects
None yet
Development

No branches or pull requests

2 participants