Performance improvement in RequestMappingInfo

[aftersss]

We are using spring-cloud-gateway integrated with spring-boot-starter-actuator, we run a load test on it(using jmeter, and my gateway program runs on a linux server with 4 cores/2GB) and found that RequestMappingInfo have some performance problem.

Scene 1: We added a Http Header Manager into jmeter with the header Accept:text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,(default value of chrome)
then I run jmeter and let my gateway program run to 2000 qps, the cpu usage is about 60%:

Scene 2: We added a Http Header Manager into jmeter with the header Accept:, then I run jmeter and let my gateway program run to 2000 qps, the cpu usage is about 35%:

As you can see, the performance is affected by the length of Accept header, maybe hackers can use this performance problem to make ddos attack easier.

I use async-profiler(https://github.com/jvm-profiling-tools/async-profiler) to profile my program, and this is the result:
aaa.txt
there are many stacktraces like the following costs too much cpu:

Total: 2777057399 (3.74%)  samples: 2766
  [ 0] java.util.HashMap.putVal
  [ 1] java.util.HashMap.put
  [ 2] org.springframework.util.MimeTypeUtils.parseMimeType
  [ 3] org.springframework.http.MediaType.parseMediaType
  [ 4] org.springframework.http.MediaType.parseMediaTypes
  [ 5] org.springframework.http.MediaType.parseMediaTypes
  [ 6] org.springframework.http.HttpHeaders.getAccept
  [ 7] org.springframework.web.reactive.accept.HeaderContentTypeResolver.resolveMediaTypes
  [ 8] org.springframework.web.reactive.result.condition.ProducesRequestCondition.getAcceptedMediaTypes
  [ 9] org.springframework.web.reactive.result.condition.ProducesRequestCondition.access$000
  [10] org.springframework.web.reactive.result.condition.ProducesRequestCondition$ProduceMediaTypeExpression.matchMediaType
  [11] org.springframework.web.reactive.result.condition.AbstractMediaTypeExpression.match
  [12] org.springframework.web.reactive.result.condition.ProducesRequestCondition.lambda$getMatchingCondition$0
  [13] org.springframework.web.reactive.result.condition.ProducesRequestCondition$$Lambda$656.1800605879.test
  [14] java.util.Collection.removeIf
  [15] org.springframework.web.reactive.result.condition.ProducesRequestCondition.getMatchingCondition
  [16] org.springframework.web.reactive.result.method.RequestMappingInfo.getMatchingCondition
  [17] org.springframework.web.reactive.result.method.RequestMappingInfoHandlerMapping.getMatchingMapping
  [18] org.springframework.web.reactive.result.method.RequestMappingInfoHandlerMapping.getMatchingMapping
  [19] org.springframework.web.reactive.result.method.AbstractHandlerMethodMapping.addMatchingMappings
  [20] org.springframework.web.reactive.result.method.AbstractHandlerMethodMapping.lookupHandlerMethod
  [21] org.springframework.web.reactive.result.method.AbstractHandlerMethodMapping.getHandlerInternal
  [22] org.springframework.web.reactive.handler.AbstractHandlerMapping.getHandler
  [23] org.springframework.web.reactive.DispatcherHandler.lambda$handle$0
  [24] org.springframework.web.reactive.DispatcherHandler$$Lambda$653.1915333739.apply
  [25] reactor.core.publisher.FluxConcatMap$ConcatMapImmediate.drain
  [26] reactor.core.publisher.FluxConcatMap$ConcatMapImmediate.onSubscribe
  [27] reactor.core.publisher.FluxIterable.subscribe
  [28] reactor.core.publisher.FluxIterable.subscribe
  [29] reactor.core.publisher.FluxConcatMap.subscribe
  [30] reactor.core.publisher.MonoNext.subscribe
  [31] reactor.core.publisher.MonoSwitchIfEmpty.subscribe
  [32] reactor.core.publisher.MonoFlatMap.subscribe
  [33] reactor.core.publisher.MonoFlatMap.subscribe
  [34] reactor.core.publisher.MonoDefer.subscribe
  [35] reactor.core.publisher.MonoDefer.subscribe
  [36] reactor.core.publisher.MonoPeekTerminal.subscribe
  [37] reactor.core.publisher.MonoDefer.subscribe
  [38] reactor.core.publisher.MonoPeekTerminal.subscribe
  [39] reactor.core.publisher.MonoFlatMap$FlatMapMain.onNext
  [40] reactor.core.publisher.Operators$MonoSubscriber.complete
  [41] reactor.core.publisher.MonoZip$ZipCoordinator.signal
  [42] reactor.core.publisher.MonoZip$ZipInner.onNext
  [43] reactor.core.publisher.Operators$ScalarSubscription.request
  [44] reactor.core.publisher.MonoZip$ZipInner.onSubscribe
  [45] reactor.core.publisher.MonoJust.subscribe
  [46] reactor.core.publisher.Mono.subscribe
  [47] reactor.core.publisher.MonoZip.subscribe
  [48] reactor.core.publisher.MonoFlatMap.subscribe
  [49] reactor.core.publisher.MonoDefer.subscribe
  [50] reactor.core.publisher.MonoDefer.subscribe
  [51] reactor.core.publisher.MonoDefer.subscribe
  [52] reactor.core.publisher.MonoDefer.subscribe
  [53] reactor.core.publisher.MonoDefer.subscribe
  [54] reactor.core.publisher.MonoDefer.subscribe
  [55] reactor.core.publisher.MonoDefer.subscribe
  [56] reactor.core.publisher.MonoDefer.subscribe
  [57] reactor.core.publisher.MonoDefer.subscribe
  [58] reactor.core.publisher.MonoOnErrorResume.subscribe
  [59] reactor.core.publisher.MonoOnErrorResume.subscribe
  [60] reactor.core.publisher.MonoOnErrorResume.subscribe
  [61] reactor.core.publisher.Mono.subscribe
  [62] reactor.core.publisher.MonoIgnoreThen$ThenIgnoreMain.drain
  [63] reactor.core.publisher.MonoIgnoreThen.subscribe
  [64] reactor.core.publisher.MonoPeekFuseable.subscribe
  [65] reactor.core.publisher.MonoPeekTerminal.subscribe
  [66] reactor.ipc.netty.channel.ChannelOperations.applyHandler
  [67] reactor.ipc.netty.http.server.HttpServerOperations.onHandlerStart
  [68] reactor.ipc.netty.channel.ContextHandler$$Lambda$633.1255094246.run
  [69] io.netty.util.concurrent.AbstractEventExecutor.safeExecute
  [70] io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks
  [71] io.netty.channel.epoll.EpollEventLoop.run
  [72] io.netty.util.concurrent.SingleThreadEventExecutor$5.run
  [73] java.lang.Thread.run

I found that the key point is that MediaType.parseMediaType is invoked too many times(more than 400) in one http request, this will cost too much cpu, so I change the order of the code, and test it again(also run to 2000 qps), this time the cpu usage is no more than 35%, the cpu usage will not be affected by the length of Accept header any more.

[jhoeller]

Would it make sense to immediately return null after each individual condition check even?

[aftersss]

Would it make sense to immediately return null after each individual condition check even?

I think it make sense, and the modification is committed.

[rstoyanchev]

1. This change puts pattern matching ahead of produces/consumes condition? Do we not want it the other way around?

2. For WebFlux we can cache the parsed Accept header in the ReadOnlyHttpHeaders wrapper based on the changes made in ce7278a. @bclozel any reason not to cache Accept header? I don't see what the downside could be.

[bclozel]

1. I agree. Consume/produce conditions are less likely to be on controller handlers, so they can often avoid doing more work by short-circuiting before the path (which is often or always there). We should keep methods, produce/consume and then path.

2. This is a good choice to consider. @aftersss would you like to amend your PR to reflect that? Overriding the getAccept method in ReadOnlyHttpHeaders and caching the result (as it's already done for the "Content-Type" should do it.

@rstoyanchev @jhoeller as an FYI, I've got an alternate MediaType parser implementation which consumes less CPU and memory - but so far the caching approach works pretty well.

[rstoyanchev]

Thanks @bclozel. I've marked this 5.1.6 since the suggested changes are straight-forward. Maybe the alternate MediaType implementation could target 5.2.

@aftersss unless I hear from you that you're reworking the PR, I will process this later today so there is a chance to test the outcome ahead of next week's 5.1.6 release.

[aftersss]

Thanks @bclozel. I've marked this 5.1.6 since the suggested changes are straight-forward. Maybe the alternate MediaType implementation could target 5.2.

@aftersss unless I hear from you that you're reworking the PR, I will process this later today so there is a chance to test the outcome ahead of next week's 5.1.6 release.

OK, I have done this change.

[rstoyanchev]

I've processed the PR + some extra polish. Note that I reversed the order so that patterns are after consumes and produces. Both are now cached for WebFlux. Also I've created #22644 to make further optimizations for 5.2 where Spring MVC should catch up on caching request values.

@aftersss it would be great if you could confirm 5.1.6.BUIlD-SNAPSHOT against the baseline numbers you have.