Closed spring-projects-issues closed 6 years ago
spring-projects-issues
commented
6 years ago Juergen Hoeller commented
I'm rolling this into my bunch of commits coming up here, at least the part where Conventions lazily retrieves the shared ReactiveAdapterRegistry. Whether we could revise Conventions to not require loading of so many Reactor classes is a separate question (Rossen Stoyanchev)... but then if Reactor is actually on the classpath, I suppose it'll be used at some point anyway?
spring-projects-issues
commented
6 years ago Andy Wilkinson commented
I suppose it'll be used at some point anyway?
Yes. If you're using WebFlux, WebFluxConfigurationSupport.webFluxAdapterRegistry() will trigger the loading of all the Reactor classes as it creates a new ReactiveAdapterRegistry.
spring-projects-issues
commented
6 years ago Rossen Stoyanchev commented
The RecativeAdapterRegistry API was updated once before (for Spring Data, #19468) to not depend on the presence of Reactor. Of course, if Reactor is present, Reactor classes are loaded in the constructor.
For WebFlux the registry is necessary to work with reactive types in controller method signatures, so could be deferred but very likely to be used although there could be exceptions (e.g. functional endpoints). For Spring MVC, RequestMappingHandlerAdapter declares it as a field, but it will only be used with reactive types as return values in controller methods.
spring-projects-issues
commented
6 years ago Andy Wilkinson commented
Thanks, Rossen. I'm in favour of deferring as much as is reasonably possible. It perhaps makes most sense in the functional endpoint case where one reason to use them is that they should perform better (no reflection, no annotation introspection, etc). Deferring in the MVC case perhaps makes sense too. It would mean that the price is only paid when there is a controller with a Reactor return type. Should I open a separate issue or can this one continue to be used?
spring-projects-issues
commented
6 years ago Juergen Hoeller commented
Alright, I'll have a look at the web module usage of ReactiveAdapterRegistry, considering a backport to 5.0.8 (like for the Conventions refinement) as well.
spring-projects-issues
commented
6 years ago Andy Wilkinson commented
Thanks, Juergen. Both for this issue and the various other changes you've made for us recently.
spring-projects-issues
commented
6 years ago Juergen Hoeller commented
Andy Wilkinson, after revisiting all usage there, access to ReactiveAdapterRegistry seems as deferred as reasonably possible already...
In the functional web case, we don't ever instantiate a ReactiveAdapterRegistry to begin with. In the annotation-based case (both for WebFlux and for MVC), we need the registry to identify supported reactive return types, and the actual registry is configurable... so it'd be rather artificial to defer access to the registry.
Trying to separate identification and adaptation there seems like a micro-optimization which would create complexity without much of a benefit, so I'd rather leave things as-is. If Reactor is present on the classpath, it seems reasonable to register its adapters once annotation-based web endpoint processing is set up.
spring-projects-issues
commented
6 years ago Rossen Stoyanchev commented
Just one note. ReactiveAdapterRegistry indeed is not instantiated when bootstrapping the server manually. In the context of a Boot application however, applications declare RouterFunction beans that in turn rely on RouterFunctionMapping declared in WebFluxConfigurationSupport that also creates a ReactiveAdapterRegistry.
Andy Wilkinson opened SPR-16981 and commented
I've been looking at the performance of
ConfigurationClassPostProcessor, particularly the overhead that it adds when are no full or lite configuration classes in the context. As part of this, I've observed that adding Reactor to the classpath has a large impact. Without Reactor on the classpath I seepostProcessBeanDefinitionRegistrytake 10-20ms. With Reactor on the classpath it can take as much as 100ms to do the same thing (add 0 bean definitions).The cost is incurred when
ConfigurationClassUtilsis loaded as it, ultimately, leads to a couple of hundred Reactor classes being loaded. Here's the beginning of the-verbose:classoutput:The javadoc on
ReactiveAdapterRegistry.getSharedInstance()indicates that the instance is lazily built. This is undone byConventionswhich stores the shared instance in a static field, thereby triggering creation of the instance as soon as the class is loaded.The registry is only used in
getVariableNameForParameterandgetVariableNameForReturnType. In the application that I have been examining, neither method is called during startup. Perhaps the initialisation of the registry could be deferred until it's needed?Beyond deferring the initialisation, I wonder if there's something that can be done to reduce the impact when it is eventually needed? The cost in terms of time and metaspace usage of loading 250 classes seems rather high.
Affects: 5.0.7
Issue Links:
20766 CPU utilization too high for a simple server
Referenced from: commits https://github.com/spring-projects/spring-framework/commit/b68e692854d4494c141259c924c667c21784c3eb, https://github.com/spring-projects/spring-framework/commit/3e64388b20e4f885b29f388e3e5bca55cd1a4cec