Spring Avoid instanceof GenericHttpMessageConverter checks

As found by @dreis2211, instanceof GenericHttpMessageConverter checks slowdown significantly the iteration on converters in Spring MVC.

This commit introduces a default boolean isGeneric() method in HttpMessageConverter and a related override in GenericHttpMessageConverter in order to avoid this check.

This change increases the throughput of Techempower JSON benchmark by 6.38% and of a benchmark performed with hey (10s after 60s of warmup) by 5.5%.

@jhoeller @rstoyanchev @poutsma The gains are significant so I would would like to propose the inclusion of theses changes in Spring Framework 6. Not sure if that would be ok on 5.3.x or not. Please let me know what you think about it.

Comment From: sdeleuze

Additional data points from my hey benchmarks: - Original throughput with default converters : 142169 req/sec - Throughput with default converters and produces = MediaType.APPLICATION_JSON_VALUE : 145944 req/sec - Throughput with a single converter configured : 150544 req/sec - Throughput with default converters and this PR: 149985 req/sec

So those figures seems to confirm that this change improves significantly the efficiency of the iteration on converters.

Comment From: sdeleuze

After discussing with @jhoeller it seems we could even consider inclusion in 5.3.x.

Comment From: dreis2211

Without confirming the improvements yet, the type pollution agent is still producing this output, because of the checkcast.

--------------------------
Type Pollution Statistics:
--------------------------
1:  org.springframework.http.converter.json.MappingJackson2HttpMessageConverter
Count:  192143
Types:
    org.springframework.http.converter.GenericHttpMessageConverter
    org.springframework.http.converter.HttpMessageConverter
Traces:
    org.springframework.web.servlet.mvc.method.annotation.AbstractMessageConverterMethodProcessor.getProducibleMediaTypes(AbstractMessageConverterMethodProcessor.java:387)
        class: org.springframework.http.converter.GenericHttpMessageConverter
        count: 96087
    org.springframework.web.servlet.mvc.method.annotation.AbstractMessageConverterMethodProcessor.getProducibleMediaTypes(AbstractMessageConverterMethodProcessor.java:385)
        class: org.springframework.http.converter.HttpMessageConverter
        count: 96056

So from that point of view, nothing really changed and potentially there could be still a scaling problem caused by https://bugs.openjdk.org/browse/JDK-8180450. But given that a simple boolean check is faster than an instanceof and that less activity is happening on the internal cache potentially, I'm not surprised to see some improvements here.

Comment From: sdeleuze

Notice we need to find a way to fix the broken test RequestResponseBodyMethodProcessorTests#resolveArgumentTypeVariableWithNonGenericConverter :

class jdk.proxy3.$Proxy29 cannot be cast to class org.springframework.http.converter.GenericHttpMessageConverter (jdk.proxy3.$Proxy29 is in module jdk.proxy3 of loader 'app'; org.springframework.http.converter.GenericHttpMessageConverter is in unnamed module of loader 'app')
java.lang.ClassCastException: class jdk.proxy3.$Proxy29 cannot be cast to class org.springframework.http.converter.GenericHttpMessageConverter (jdk.proxy3.$Proxy29 is in module jdk.proxy3 of loader 'app'; org.springframework.http.converter.GenericHttpMessageConverter is in unnamed module of loader 'app')
    at org.springframework.web.servlet.mvc.method.annotation.AbstractMessageConverterMethodArgumentResolver.readWithMessageConverters(AbstractMessageConverterMethodArgumentResolver.java:176)
    at org.springframework.web.servlet.mvc.method.annotation.RequestResponseBodyMethodProcessor.readWithMessageConverters(RequestResponseBodyMethodProcessor.java:163)
    at org.springframework.web.servlet.mvc.method.annotation.RequestResponseBodyMethodProcessor.resolveArgument(RequestResponseBodyMethodProcessor.java:136)

Calling ProxyFactory#getProxy(java.lang.Class<T>, org.springframework.aop.TargetSource) on an interface with a default method seems not working for this use case, not sure why yet.

Comment From: sdeleuze

Test fixed via this change which looks ok to me based on what was discussed in #29438.

Comment From: sdeleuze

Without confirming the improvements yet, the type pollution agent is still producing this output, because of the checkcast.

@dreis2211 Notice I initially tried a more elegant/effective fix that would use casting only in the constructor by adding a new List<GenericHttpMessageConverter<?>> genericMessageConverters field in addition to the existing List<HttpMessageConverter<?>> messageConverters one, but I stopped because the order of the converters matters so we can't iterate first on the GenericHttpMessageConverter and after on the non generic HttpMessageConverter.

Given the good figures of the changes of this PR, I think my proposal is in practice effective enough, if you see a reasonable way to avoid per request casting, feel free to share.

Comment From: sdeleuze

Based on @poutsma feedback, I am testing various alternative implementations without impact on the public APIs.

Comment From: sdeleuze

After increasing significantly (10x) the number of runs of my benchmarks with both Techempower and hey, I am not able to reproduce a significant difference between main and the isGeneric property variant anymore, so I will just close this PR unmerged.