I was doing some flame graph analysis and found that ReadOnlyHttpHeaders.entrySet() was a small hotspot in my codebase. I looked into its implementation and found that some care was taken in the past to fully copy over the entrySet to into a new and properly ordered set. The main hotspot in my specific codebase was in BodyInserterRequest.writeTo() which was called every time I used the reactive WebClient. I found that the entrySet() could be avoided and replaced with a forEach with putIfAbsent. The putIfAbsent is particularly useful since in the primary implementation I found it has to hash/normalize keys only once instead of twice like it was doing currently:
https://github.com/spring-projects/spring-framework/blob/b23316302d578694c6915d5fc9ef24b571110ffc/spring-core/src/main/java/org/springframework/util/LinkedCaseInsensitiveMap.java#L209-L223
In order to make sure the fast path is taken, I had to make sure the whole call chain avoided the Map class' default implementations (which use the slow entrySet() or a naive putIfAbsent). this allowed us to get to LinkedCaseInsensitiveMap's implementations. The forEach is optimized in LinkedHashMap as well. This foregoes the need to create entrySets and iterators (all with immutability) which saves memory allocations. Here is a stack trace from a breakpoint in LinkedCaseInsensitiveMap.putIfAbsent() (note how the fast path is taken):
Some other minor optimizations were included: presizing list in MultiValueMapAdapter.addAll, use forEach in implementation of MultiValueMapAdapter.addAll (this avoids that slow entrySet path)
ASIDE: I think I found a bug in the ReadOnlyHttpHeaders.entrySet(). It appears that it leaks a mutable value list. See the following code:
HttpHeaders src = new HttpHeaders();
src.set("foo", "bar");
src = HttpHeaders.readOnlyHttpHeaders(src);
src.entrySet().iterator().next().getValue().add("blah") // this should throw since ["bar"] is should be unmodifiable.
assertThat(src.get("foo").size()).isEqualTo(2); // should really be 1?
That code currently works, but should it? I have a fix, but I'll save that for a separate PR if you want.
I can also look into adding forEach implementations in things like NettyHeadersAdapter later.
Comment From: yuzawa-san
Here are the results of my stress test. This is a portion of BodyInserterRequest.writeTo(), but the savings can be realized all over the place: DefaultRequestBodyUriSpec.init(), AbstractServerResponse.copy(), ReactorClientHttpRequest.apply(), and everywhere else in user code where putAll or addAll is used on the headers.
before memory:
after memory:
before cpu:
after cpu:
Comment From: yuzawa-san
@rstoyanchev would it be possible to triage this? let me know if anything else is needed.
Comment From: bclozel
Thanks for this PR @yuzawa-san and sorry about the delay.