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

Optimize AbstractAutowireCapableBeanFactory.populateBean(String, RootBeanDefinition, BeanWrapper) to avoid redundant Java Bean introspection [SPR-16918] #21457

Closed
spring-issuemaster opened this Issue Jun 7, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Jun 7, 2018

Andy Wilkinson opened SPR-16918 and commented

In a typical Spring Boot app, a significant amount of the startup cost is due to org.springframework.beans.CachedIntrospectionResults.forClass(Class<?>). There's a direct cost in terms of the CPU time taken for introspection and a secondary cost due to increased memory usage and GC pressure.

In the vast majority of cases, the introspection results are retrieved beneath AbstractAutowireCapableBeanFactory.populateBean(String, RootBeanDefinition, BeanWrapper) so that it can retrieve the property descriptors and then filter them. If I short-circuit populateBean so that it returns early if the bean definition's property values are empty, an app that took 2.7 seconds to start then starts in as little as 2.3 seconds.

The short circuiting loses two pieces of functionality:

  1. The opportunity for InstantationAwareBeanPostProcessors to add property values
  2. @Required support

Boot itself doesn't make use of either of these so the current ~400ms cost is a high price to pay. I'd like to explore the possibility of lowering or removing this cost.

As far as I can tell only one of the built-in post-processors, RequiredAnnotationBeanPostProcessor, uses the property descriptors, and it only does so if the bean definition hasn't been marked to skip the required check. One possibility may be to retrieve the property descriptors lazily and for Boot to somehow mark its bean definitions with skipRequiredCheck.


Affects: 5.0.6

Issue Links:

  • #21663 BeanDefinition-aware BeanPostProcessors should clear cache in case of bean definition reset

Referenced from: commits 81cb740

0 votes, 6 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 7, 2018

Andy Wilkinson commented

The short circuiting loses two pieces of functionality

As revealed by running a bigger app this is an understatement. The short-circuiting also loses @Autowired field injection which Boot still uses in a couple of places. Instead, I've removed the short-circuiting and instead replaced the retrieval of the filtered property descriptors with the following:

PropertyDescriptor[] filteredPds = new PropertyDescriptor[0];

With this change in place the bigger app that uses Boot's Actuator starts in 3.5s vs 4.2s. Given the larger number of beans in this app a larger improvement in startup time is to be expected.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2018

Dave Syer commented

So, to be clear, the only thing we lose with the empty property descriptors is support for @Required processing? If so, it seems like a no-brainer - we should make @Required processing optional, or even deprecate it completely.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 11, 2018

Phil Webb commented

It might be a bit more complex because the PropertyDescriptor arrays is passed to InstantiationAwareBeanPostProcessor.postProcessPropertyValues. So even if framework only need them for @Required processing, it's possible external users need them for something else. Having said that, there is this comment:

* <p><b>NOTE:</b> This interface is a special purpose interface, mainly for
* internal use within the framework. It is recommended to implement the plain
* {@link BeanPostProcessor} interface as far as possible, or to derive from
* {@link InstantiationAwareBeanPostProcessorAdapter} in order to be shielded
* from extensions to this interface.

so perhaps we can replace that method with one that is given a Supplier<PropertyDescriptor[]>

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 16, 2018

Juergen Hoeller commented

I currently have a new postProcessProperties callback with just a PropertyValues argument (which is needed for checking external overrides on autowired properties) and no PropertyDescriptors needed. The injection processors use this callback now, opting out of postProcessPropertyValues through returning false from postProcessProperties. Applying this optimization to a few other common post-processors as well and not registering RequiredAnnotationBeanPostProcessor by default anymore skips that PropertyDescriptor initialization and filtering step in most scenarios now, while leaving it available for custom InstantiationAwareBeanPostProcessor implementations still.

I've also deprecated @Required and RequiredAnnotationBeanPostProcessor now, in favor of constructor injection (or afterPropertiesSet checks) for such required settings. This justifies the removal from the set of default processors since we'll simply not assert on those properties anymore but otherwise keep an @Required-declaring application working. And RequiredAnnotationBeanPostProcessor may still be explicitly declared when really needed.

Allowing for one more round of fine-tuning the new callback and its semantics, I'll try to have this in master by Monday.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 28, 2018

Juergen Hoeller commented

A revised variant is finally in master now, with postProcessProperties superseding postProcessPropertyValues completely since postProcessProperties has a PropertyValues return value in this variant. The postProcessPropertyValues callback is deprecated along with RequiredAnnotationBeanPostProcessor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.