Spring Optimize initial size in ContentCachingRequestWrapper when contentCacheLimit set

To avoid over-allocating the initial buffer for content caching: 1. If the content size is known and is smaller than the content limit, use that 2. If the content size is unknown use the default initial buffer size (or content limit if smaller) and allow it to grow as needed

This allows for scenarios where limiting behavior is desired to avoid overly large caching but at the same time the majority of requests are not near that limit. For example, allowing a maximum of 1MB content caching but the majority of requests are in the single digit KB size. Rather than allocate the worst case 1MB byte arrays per request these can be scaled in to be more appropriately sized.

Comment From: ryanrupp

Ran into this because we're using this similar to AbstractRequestLoggingFilter except slightly different formatting (just wanted to capture the contents) and we're trying to capture up to a larger size (say 1MB) but at the same time safeguard ourselves from overly large requests. These large requests are an edge case though so we want to avoid eagerly allocating the content limit size because this would over-allocate quite a bit in most cases. Alternatively, there could be another overloaded constructor that takes the buffer initial size but this seemed like maybe exposing too much implementation detail.

Only downside really of this is that on the unknown size requests, the init buffer will now just be the smaller of 1024 or the content size limit. In theory there's someone that had the limit set to something larger and knew most of their requests would end up that size and now there's some level of byte buffer resizing that takes place. Maybe use Spring's FastByteArrayOutputStream if this is a concern. AbstractRequestLoggingFilter currently defaults the content size limit to 50 bytes so it would always take that over the 1024 default for unknown sizing.

Comment From: bclozel

I think this change is a good tradeoff, avoiding over-allocation in many cases. If this change causes too much resizing, we can always consider using FastByteArrayOutputStream as pointed out by @ryanrupp . Maybe we should do that right away? With FastByteArrayOutputStream we could even drop the initial capacity value as allocating more is very efficient.

As for the tests, I'm not convinced we should use the OOM approach to validate the changes. I'd rather use ReflectionUtils to get the backing cachedContent and check its actual allocated size in the test.

Comment From: ryanrupp

@bclozel

I'm good with moving to FastByteArrayOutputStream - mainly just less familiar with this class/implementation (I know it exists) but not sure if there were any tradeoffs around its usage
I'm good with using ReflectionUtils to grab internal state for the test assertions, TBD how this changes/is necessary if using FastByteArrayOutputStream (probably still some internal state that could be confirmed on FastByteArrayOutputStream).

Will play with this when I get a chance, thanks.

Comment From: bclozel

Thanks @ryanrupp for this PR. I've polished it a bit and used the FastByteArrayOutputStream as a way to make allocations cheap and avoid the initial buffer size issue altogether. As for tests, it was not possible to reflect on ByteArrayOutputStream to test for the internal buffer size. With the latest change, it's not really necessary anymore.

This will be released with the first 6.1 release candidate.

Comment From: lonre

Hi @bclozel

can we port f83c6094362b7e16fe08a4307cbcb0015e203d23 to branch 5.3.x?

Comment From: bclozel

@lonre Is this causing significant problems in your applications? This is a light performance improvement and we don't usually backport those in maintenance branches as only bug fixes apply there.

Comment From: lonre

@bclozel Yes, according to the jfr

Spring Optimize initial size in ContentCachingRequestWrapper when contentCacheLimit set

ContentCachingRequestWrapper seems allocating much memory.

Comment From: bclozel

Thanks for your response. Allocations by the ContentCachingRequestWrapper are very much expected; here, we're just optimizing for the case where the actual response body is smaller than the default. Backporting such a change to a long-standing maintenance branch is too risky so I'm afraid upgrading to 6.1 is the best way to get this change.

Comment From: kilink

Could we reconsider aspects of this change? I think switching to FastByteArrayOutputStream is good for the case where the content size is unknown or there is no contentCacheLimit; however, when the content size is known, it seems to me to always be preferable to size the backing array precisely. I assume most usage of ContentCachingRequestWrapper will involve either calling getContentAsByteArray or getContentAsString, both of which will perform better if the backing buffer is either larger than or precisely the size of the contents.

Example scenario:

We have a 10KB request payload, and the content size is known. If we pass this size to the FastByteArrayOutputStream, no "collation" is needed if the user then calls getContentAsString or getContentAsByteArray. If we go with the current approach of passing in a default size of 1024, we'll end up with several buffers that must be combined (which happens in the resize method of FastByteArrayOutputStream).

In practice we do have some services that have such large request / response payloads, so I think this may be a regression in practice from simply using a ByteArrayOutputStream with a precisely sized buffer.