Repeatedly instantiating beans can trigger redundant lookups of classes and parameter annotations, and in certain applications become noticeable performance issues. This change eliminates the redundant work by a) changing MethodInvoker.prepare() to only lookup targetClass when it's missing, and b) changing InjectionPoint to cache the method annotations similarly to the existing caching for field annotations.
Closes: #30883
Comment From: pivotal-cla
@jengebr Please sign the Contributor License Agreement!
Click here to manually synchronize the status of this Pull Request.
See the FAQ for frequently asked questions.
Comment From: pivotal-cla
@jengebr Thank you for signing the Contributor License Agreement!
Comment From: jhoeller
Which usage of MethodInvoker are you seeing as a hotspot, actually? We usually only call prepare() once for our internally prepared MethodInvoker instances, so I wonder how that optimization would make a difference. If you'd like to optimize the initial preparation of a static method there, I'd rather recommend setting targetClass+targetMethod instead of staticMethod. Setting targetClass+staticMethod is not an intended combination, and in case of a reinitialization attempt, we mean to let staticMethod override any configuration set in targetClass/targetMethod before.
As for the InjectionPoint part, MethodParameter.getParameterAnnotations() itself caches the annotation array. Since the MethodParameter instance itself is always the same there, I don't see a benefit in locally caching the annotation array in InjectionPoint as well. If something is going wrong with the existing cache intentions that we have in MethodParameter, we should rather aim to fix it there.
Comment From: jhoeller
Oh, and thanks for your optimization attempts there, that's much appreciated! I'd be happy to fine-tune the caching that we have there already etc. Any changes that make a measurable difference for you are worth investigating, finding out why existing caching does not cover it already. It's just that the right place to address such shortcomings is not always obvious (even to us).
Comment From: jengebr
Thanks for reviewing this! Our apps run 5.2.x so it's always possible this is comes from a version difference.
And you summarized the situation quite well with this:
It's just that the right place to address such shortcomings is not always obvious (even to us).
Re: InjectionPoint, I see what you're saying, and I'll look upwards through the call tree to see what else may be at fault.
Re: MethodInvoker, I'm not able to identify the specific bean configuration(s) causing this in production, but reinitialization is an interesting case that I hadn't considered, and it might explain all of this. It shouldn't be happening, but it's conceivable.
Detailed stacks are below.
The hot stack for MethodInvoker is:
java.lang.Class.forName0
java.lang.Class.forName
org.springframework.util.ClassUtils.forName
org.springframework.beans.factory.config.MethodInvokingBean.resolveClassName
org.springframework.util.MethodInvoker.prepare
org.springframework.beans.factory.config.MethodInvokingFactoryBean.afterPropertiesSet
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.lambda$invokeInitMethods$5
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory$Lambda.run
java.security.AccessController.doPrivileged
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.invokeInitMethods
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.initializeBean
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.doCreateBean
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBean
<proprietary BeanFactory subclass>.createBean
org.springframework.beans.factory.support.AbstractBeanFactory.lambda$doGetBean$1
org.springframework.beans.factory.support.AbstractBeanFactory$Lambda.getObject
<proprietary request scope subclass>.createBean
<proprietary request scope subclass>.get
org.springframework.beans.factory.support.AbstractBeanFactory.getBean
org.springframework.beans.factory.support.BeanDefinitionValueResolver.resolveReference
org.springframework.beans.factory.support.BeanDefinitionValueResolver.resolveValueIfNecessary
org.springframework.beans.factory.support.ConstructorResolver.resolvePreparedArguments
org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod
org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance
and for InjectionPoint is:
java.lang.reflect.Method.getParameterAnnotations
org.springframework.core.MethodParameter.getParameterAnnotations
org.springframework.beans.factory.InjectionPoint.getAnnotations
org.springframework.context.annotation.ContextAnnotationAutowireCandidateResolver.isLazy
org.springframework.context.annotation.ContextAnnotationAutowireCandidateResolver.getLazyResolutionProxyIfNecessary
org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency
org.springframework.beans.factory.support.ConstructorResolver.resolveAutowiredArgument
org.springframework.beans.factory.support.ConstructorResolver.resolvePreparedArguments
org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod
org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod
org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.createBeanInstance
Comment From: jhoeller
The problem for InjectionPoint seems to be that we're not caching the DependencyDescriptor instances for prepared constructor arguments but rather just an autowiring marker there. For @Autowired fields and methods, we cache the DependencyDescriptor instances in AutowiredAnnotationBeanPostProcessor, so we should also do so in ConstructorResolver (with a specific marker subclass of DependencyDescriptor). I'll push a corresponding revision of ConstructorResolverto main, 6.0.x and 5.3.x ASAP (with a reference to #30883).
With the MethodInvoker, I suspect that your MethodInvokingFactoryBean definitions are non-singleton, in which case it would re-initialize a fresh MethodInvokingFactoryBean instance every time. You could define the MethodInvokingFactoryBean itself as a singleton and switch its singleton property to false instead, sharing the FactoryBean instance but letting it perform a fresh target method call for every getObject invocation. Also, it should make a difference here when switching from staticMethod to targetClass+targetMethod configuration since the container will cache the resolved Class argument for targetClass, even on a non-singleton definition.
Comment From: jhoeller
This should be fully superseded by my resolution for #30883 now, covering all identified hotspots.