Affects: \v5.2.9.RELEASE

CPU profiling of some fairly high HTTP traffic spring Webflux (on Tomcat) applications has shown between ~3-10% of time (in aggregate) spent in org.apache.commons.logging.LogFactory.getLog calls made from the following locations:

  • https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpRequest.java#L49
  • https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/web/method/HandlerMethod.java#L65
  • https://github.com/spring-projects/spring-framework/blob/master/spring-webflux/src/main/java/org/springframework/web/reactive/result/condition/AbstractMediaTypeExpression.java#L38
  • https://github.com/spring-projects/spring-framework/blob/master/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/HandlerMethodArgumentResolverComposite.java#L44
  • https://github.com/spring-projects/spring-framework/blob/master/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java#L62

Based on my initial inspection, some of these loggers may be unused in spring-framework code. Comments suggest in at least some of the cases that the loggers are intentionally available for use by subclasses (though I haven't seen any such usage in the spring-framework included subclasses that are in play in my specific stack traces).

I think the most typical pattern is to use static constants to refer to Loggers (since their construction is relatively expensive). Is there good reason for these Loggers to be instance members instead in these cases? If so, I wonder if it could be made configurable such that applications that aren't interested in these loggers can avoid spending time constructing them? I looked for an existing way to configure this, but wasn't able to find anything.

Comment From: jhoeller

I'm surprised that this shows as such a hotspot... In any case, we can certainly try to refine those particular spots in 5.3 still.

@rstoyanchev Any concerns here? None of those are user extension points so we should be able to refactor logger usage internally, potentially using redeclared static loggers in subclasses etc. If at all, I'd only expect potential binary compatibility issues in third-party infrastructure... but that minor remaining risk should be acceptable for 5.3 since we'll find out soon enough.

Being at it, I also intend to revisit logger usage in Spring MVC since similar hotspots could emerge there.

Comment From: jhoeller

A quick analysis reveals that the loggers in AbstractMediaTypeExpression and HandlerMethodArgumentResolverComposite are unused and declared on non-public classes, so we can simply drop them even in the 5.2.x and 5.1.x lines. The loggers in AbstractServerHttpRequest/Response are only really used for niche purposes and can probably be dropped as well (since the static rsWriteLogger covers the debugging need already). HandlerMethod and its subclasses are a bit more involved but also entirely in our hands, allowing for internal refactoring there.