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

Revisit @Bean introspection between @Configuration classes and 'lite' beans [SPR-17206] #21739

Closed
spring-projects-issues opened this issue Aug 23, 2018 · 8 comments
Assignees
Labels
in: core type: task
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Aug 23, 2018

Dave Syer opened SPR-17206 and commented

There is a lot of evidence that ConfigurationClassPostProcessor is bad for startup time. There is also some evidence that part of the problem is lite bean configuration - we have to scan all methods of all bean definition classes looking for @Bean. To investigate this I made a version of ConfigurationClassPostProcessor that only uses classes from spring.components. It is much faster when there are no @Configuration classes.

The rationale for introducing lite beans was IIRC so that you wouldn't pay the cost of CGLib proxying. But that is so much faster now than it was when we invented @Configuration I don't believe it adds up any more - the cost of processing annotations is much higher. And we have to recursively search all nested classes of all bean definitions. Ugh.

If lite beans were optional, Spring Boot could switch them off, and we can flush out all the usages (which are probably mostly accidental at this point).


Affects: 5.0.8

Issue Links:

  • #21136 JUnit Jupiter @Nested class cannot share enclosing class's ApplicationContext if nested class is deemed to be a configuration candidate
  • #21674 Java 10: "Illegal method name" when test functions in Kotlin contain spaces in name
  • #19660 Spring internal configuration classes can no longer use @EventListener
  • #15238 Do not consider all @Component instances as @Configuration candidates
  • #12837 Automatically detect and register nested @Configuration classes
  • #15163 Consider @Import classes as lite @Configuration
  • #21379 A lite configuration class's member classes are processed when it's imported but not when it's registered directly
  • #16391 @ComponentScan should get processed without @Configuration as well
  • #21945 Add bean definition attribute for ConfigurationClassPostProcessor to skip "lite" bean detection

2 votes, 8 watchers

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 23, 2018

Stéphane Nicoll commented

I like this idea, we could configure this automatically in the next feature release of Spring Boot before eventually deprecating/removing the feature.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 23, 2018

Juergen Hoeller commented

The original motivation was not the cost of CGLIB subclass generation but rather the limitations that come with it: e.g. no final on the containing class or on an @Bean method, and the requirement for runtime class generation into the current ClassLoader to be possible in the first place. Frankly, with recent environment restrictions like in Jigsaw and Graal, the latter is a more relevant issue than it was before. For a simple factory method case without any @Bean cross-references, the CGLIB subclass is completely artificial and does not add any value. I believe that we need such a plain mechanism and cannot simply take the 'lite' variant away. This is not about a configuration class variant really but rather about factory methods to be added to other kinds of aggregator components where additional beans are to be exposed to the container. It also goes nicely with interface-based proxies for applications with no use of CGLIB at all, and it is directly equivalent to the good XML factory-method attribute which is just a dispatch to an unmodified factory method as well. For the record, CDI has a mechanism that is semantically very close to our lite-style @Bean methods; CDI does not have an equivalent to our cross-reference stuff on @Configuration classes at all.

That said, simple @Bean methods were only ever meant to be supported on top-level components, just like @EventListener. The simple factory method variant being searched on nested classes seems like a side effect from the @Configuration semantics where nested classes are auto-imported as configuration candidates, never intended to be searched for other purposes. Restricting the applicability of simple factory methods to their original intention should provide us with introspection performance in the same category as @EventListener and not too far from our common autowiring annotations either, moving it away from a hotspot position. I would much prefer such tightening over different modes of introspection, and this should be easily doable for 5.1 still.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 23, 2018

Juergen Hoeller commented

FWIW, we also got #21674 as a recent case for where a pointlessly created CGLIB proxy turned out as a showstopper, in this case with Kotlin.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Aug 24, 2018

Sam Brannen commented

Juergen Hoeller, if you do any work on this issue, please keep in mind that this topic is related to #21136.

@spring-projects-issues
Copy link
Collaborator Author

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

Dave Syer commented

Maybe we could have an explicit option to only load beans from spring.components, and extend that index to include lite beans for people that need them (or are using them inadvertently)? Cutting out the recursive search through all bean classes and nested classes is the main outcome I was seeking TBH.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Juergen Hoeller commented

I've restricted the nested class search to @Configuration classes and @Component classes with configuration indicators, covering #21136's conflict for nested test classes and undoing 5.1 RC1's #21379 towards consistently not finding such lite configuration classes (neither for imports nor for direct registrations).

For the above-mentioned two issues, this is a worthwhile revision in its own right. Beyond that, whether it covers a lot of ground in terms of startup performance is rather doubtful: The startup overhead for nested classes only really shows for top-level classes with configuration indicators since otherwise we wouldn't search the nested classes to begin with.

As for searching top-level classes themselves for @Bean methods, this is in the same performance ballpark as our search for @EventListener methods. Cutting that out would affect a lot more people than restricting the nested class search. Also, see #15163 and #16391 for 4.0 era requests to find our configuration indicators more broadly than before.

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Dave Syer commented

It covers some ground in terms of startup performance. A micro app with no @Bean methods now uses 2.2% of startup time on ConfigurationClassPostProcessor compared to 3.7% in 5.1.0.RC3. All the time is now spent in ConfigurationClassUtils.isLiteConfigurationCandidate(), where before it was split between that and ConfigurationClassUtils.hasNestedConfigurationClass(). I expect that larger apps fare better proportional to the number of bean definitions Thanks.

The decision about ConfigurationClassUtils.isLiteConfigurationCandidate() can be made at compile time, so maybe an index of actual lite configuration classes is still a good idea. Should I open another issue?

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Sep 12, 2018

Juergen Hoeller commented

ConfigurationClassUtils.hasNestedConfigurationClass() was effectively a performance regression in 5.1 RC1 which hopefully Andy Wilkinson doesn't mind that I resolved the other way (consistently not searching nested classes there) now.

As for a more extensive annotation index, this is indeed worth pursuing. Our current components indexer is rather simple, after all, and certainly has the potential to become more sophisticated (towards an indication for annotated methods in particular).

@spring-projects-issues spring-projects-issues added in: core type: task labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 5.1 GA milestone Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core type: task
Projects
None yet
Development

No branches or pull requests

2 participants