Skip to content

Comprehensively cache annotated methods for interfaces and superclasses [SPR-16675] #21216

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

Closed
spring-projects-issues opened this issue Mar 31, 2018 · 9 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Mar 31, 2018

Juergen Hoeller opened SPR-16675 and commented

AnnotationUtils has had an internal annotated interface cache (#12286) as well as a general lookup cache (#16501) for a while. However, recent performance benchmarks showed that further significant gain can be achieved by turning the interface cache into an annotated base type cache, caching metadata about superclasses as well... in particular about non-annotated base classes that are never worth searching. We're also narrowing potential base type lookups to a set of candidate methods that carry any annotations to begin with, avoiding an often NoSuchMethodException-triggering Class.getMethod lookup for a base method in favor of matching against the candidate Method set from our cache.


Affects: 5.0.4

Issue Links:

0 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

Christoph Dreis commented

You don't happen to have any numbers on how significant the improvement is by any chance?

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Not in the context of a Boot app yet. In local microbenchmarks involving annotation introspection across class hierarchies and interfaces with unrelated annotations, it was 2-5x as fast. However, this is very dependent on the concrete bean classes in a setup and how expensive the activated annotation introspection actually is (e.g. @EnableCaching)... That said, whenever there are annotated interfaces or class hierarchies involved, a noticeable benefit is very likely.

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

As of 5.0.5, we're also clearing our AnnotationUtils cache along with other core reflection caches at the end of the context refresh now, assuming that most of the affected metadata (in particular the annotated base type cache but also the find annotation cache) won't be needed after the singleton instantiation phase anymore. The corresponding AnnotationUtils.clearCache() method is also available in 4.3.15 but not called there by default.

@spring-projects-issues
Copy link
Collaborator Author

David commented

I think there will be more improvement:

Set<Class<?>> classes = new LinkedHashSet<Class<?>>(ClassUtils.getAllInterfacesForClassAsSet(targetClass));
		classes.add(targetClass);
		for (Class<?> clazz : classes) {
			Method[] methods = ReflectionUtils.getAllDeclaredMethods(clazz);
			for (Method method : methods) {
				if ((introductionAwareMethodMatcher != null &&
						introductionAwareMethodMatcher.matches(method, targetClass, hasIntroductions)) ||
						methodMatcher.matches(method, targetClass)) {
					return true;
				}
			}
		}

If we found that targetClass is a Proxy Class(eg: Proxy by JDK Proxy), then there is not need to check the method only existing is the Proxy Class (eg: Methods in org.springframework.aop.framework.Advised)

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Apr 1, 2018

David commented

When I mix use BeanNameAutoProxyCreator and AnnotationAwareAspectJAutoProxyCreator to proxy same bean cause a weird thing? I create a issue to describe it, could you help me out?

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Andy Wilkinson, there seems to be a direct reference to the AnnotatedUtils.annotatedInterfaceCache field in the DevTools Restarter (as per the comment on 129c05b)... While we may reintroduce a field of that name in deprecated form, simply holding a reference to the annotatedBeanTypeCache Map in untyped form, it's preferable to call AnnotationUtils.clearCache() for any such purposes now... also in Framework 4.3.15 and therefore possibly Boot 1.5.10, assuming that you need to clear all annotation caches.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Apr 2, 2018

Juergen Hoeller commented

Andy Wilkinson, Stéphane Nicoll, it'd be great to get a full round of Boot/Platform integration testing today still... I don't expect any regressions in real-life scenarios but there may always be strange tests relying on very specific old behavior. Since this revision (as well as #21208) is primarily there for Spring Data in a Boot setup, we need to make sure that it won't cause any trouble there... just hopefully improves startup performance a bit.

@spring-projects-issues
Copy link
Collaborator Author

Phil Webb commented

Juergen Hoeller I've update the Boot code and we managed to get at least one build to go green (we've got unrelated CI issues). Our 1.5.x build is broken currently due to DATAJPA-1309 but I don't think that's related to this change.

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

Phil Webb, Stéphane Nicoll, Andy Wilkinson, I've pushed one more pass to master, also addressing MethodIntrospector's introspection algorithm (used for @EventListener detection and similar purposes) where we start annotation introspection from the user class level as well now... avoiding the manual per-method traversal from the CGLIB subclass to its superclass (and the corresponding pressure on AnnotationUtils.annotatedBaseTypeCache), like we do for AOP pointcut matching already.

Let's take it slowly tomorrow and do a full round of integration testing before we go live here. I don't expect 5.0.5 and 4.3.15 before late European afternoon at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants