Minimise need to iterate over a class's methods #22420
Comments
spring-projects-issues
added
the
status: waiting-for-triage
jhoeller
added
type: enhancement
|
With some hacks in place to avoid the two occurrences described above, and with
After these we move on to proxy creation in Last of all (in the case I'm currently looking at) we get to |
|
Somewhat related to this, I think there is some benefit in making the factory method that's used to create something defined using Boot supports the use of Please let me know if you'd prefer for this piece to be tracked as a separate issue. |
sbrannen
added
in: core
|
I'm about to commit an initial revision: a mechanism for bypassing method traversal for annotation introspection if possible. The As for factory method resolution, I'll tackle this in a separate revision. An initial prototype works reasonably well. We just need to handle the rare cases of |
The isCandidateClass mechanism is consistently used for a bypass check before method traversal attempts. While by default this is only bypassing standard java types, the same mechanism can be used with index metadata which indicates non-presence of certain annotations. See gh-22420
|
FWIW, that remaining part is only really trying to avoid full method traversal for pre-identified |
|
I'm about to wrap up the factory method part: Aside from some general optimizations (like consistently ignoring FWIW, for the common ASM case, it's hard to avoid full reflective initialization of the affected |
|
The change made here seems to have affected some error handling such that a package com.example.demo;
import static org.hamcrest.Matchers.instanceOf;
import static org.junit.Assert.fail;
import org.junit.Assert;
import org.junit.Test;
import org.springframework.beans.factory.BeanNotOfRequiredTypeException;
import org.springframework.context.annotation.AnnotationConfigApplicationContext;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.context.annotation.Import;
import org.springframework.scheduling.annotation.Async;
import org.springframework.scheduling.annotation.EnableAsync;
public class BeanNotOfRequiredTypeTests {
@Test
public void exceptionThrownWithBeanNotOfRequiredTypeRootCause() {
try {
new AnnotationConfigApplicationContext(JdkProxyConfiguration.class).refresh();
fail();
} catch (Throwable ex) {
while (ex.getCause() != null) {
ex = ex.getCause();
}
Assert.assertThat(ex, instanceOf(BeanNotOfRequiredTypeException.class));
}
}
@Configuration
@EnableAsync
@Import(UserConfiguration.class)
static class JdkProxyConfiguration {
@Bean
public AsyncBean asyncBean() {
return new AsyncBean();
}
}
@Configuration
static class UserConfiguration {
@Bean
public AsyncBeanUser user(AsyncBean bean) {
return new AsyncBeanUser(bean);
}
}
static class AsyncBean implements SomeInterface {
@Async
public void foo() {
}
@Override
public void bar() {
}
}
interface SomeInterface {
void bar();
}
static class AsyncBeanUser {
AsyncBeanUser(AsyncBean asyncBean) {
}
}
}The exception that's thrown with the latest 5.2.0 snapshots is the following: The exception that's thrown with 5.1.4 is the following: I think 5.1.4's behaviour is more helpful to the user. We have failure analysis in Boot for |
|
Hmm I saw a variant of this in some of our tests initially but thought I had fixed it for good. I'll have a look right now. |
|
I've pushed a revision which should fix this. Turned out to be as simple as going into our proper exception handling code path even when the arguments remain unresolved for a pre-determined factory method, not just when the factory method itself remains unresolved. |
|
Thanks. I'm seeing the same behaviour as 5.1.4 again now. |
|
The changes here haven't gone quite far enough to allow me to address this scenario from the list above:
I have a bean with ASM-based metadata that's been instantiated by Would it be possible to somehow expose |
|
Hmm we have |
|
I wasn't sure if |
|
On review, I can't find a code path where |
|
Sorry, it looks like there's some ordering in play here. Looking in the debugger, By that point, We have a BFPP at the moment and when it's called beans with standard annotation metadata have It looks like we need to move away from using a BFPP. I think that's fine as I can't see too much value in it as we only need the information that it provides once the beans have been created and we can get that in a more straightforward manner now I believe. |
|
Thanks for bearing with me. I think I've found the source of my confusion. When the bean is being created, the bean definition has been merged and is not the same as the definition that's returned when asking the bean factory for a definition by name. As a result, the factory method information is not available when examining the definition that's returned by the bean factory. |
|
Merging preserves the Or are we suffering from early merging here where the merge result is not preserved since the configuration isn't frozen yet? |
In the case I'm looking at, it's being set after the merging. Specifically, it's happening here:
That's what I needed. Thanks. I've reworked the original code so that it's no longer a BFPP and we now just look up the information on demand. This means that the earliest that it will happen is in bean post-processing by which time the bean has been instantiated and the merged bean definition has the outcome of the resolution efforts. |
Not that people often overload their |
|
Things have changed quite a bit in 2.2. We're now using the resolved factory method that's cached on the definition so overriding should not cause any problems. That said, please don't point people to |
This commit makes use of the new getResolvedFactoryMethod() method in RootBeanDefinition. See spring-projectsgh-22541
There are a number of places where Framework iterates over a class's methods. This issue is intended to identify the places where the iteration is unnecessary and can be eliminated.
ReflectionUtilskeeps a per-Class cache of declared methods. To provide any significant benefit it will be necessary to eliminate all calls toReflectionUtilsfor a particular class. It's not yet known if this is possible.AbstractAutowireCapableBeanFactory.determineTargetTypeuses the bean definition'stargetTypeto short-circuit type determination. When a bean is defined byConfigurationClassBeanDefinitionReaderprocessing a@Beanmethod, this type information is available but it is not used. If the metadata for the@Beanmethod isStandardMethodMetadata, the target type of the definition is available from the introspected method's return type. Otherwise, the name of the target type is available fromMethodMetadata.getReturnTypeName(). It appears to be possible for this type name to be stored in the definition and then be used to load the named class whengetTargetType()is invoked.When processing a
ConfigurationClassBeanDefinition,ConstructorResolver.instantiateUsingFactoryMethodcallsReflectionUtils.getAllDeclaredMethodsto find theMethodfor the@Beanmethod for which the bean definition was created. When the definition was created fromStandardMethodMetadata, I think this method will already have been available and could, perhaps, have been stored in the definition'sresolvedConstructorOrFactoryMethodfield. For ASM-based metadata, the method's name is available and, with some changes to the metadata, it looks like its parameter types could be too. This may then allow the method to be identified directly without the need to iterate over all methods.The text was updated successfully, but these errors were encountered: