Optimize initial size in ContentCachingRequestWrapper when contentCacheLimit set

[ryanrupp]

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.

[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.

[ryanrupp]

This is a bit awkward to test without exposing some internals in a package private method or something (maybe reflection to grab the byte array size). Basically, using a really large content limit before this change could cause an OOM Error if you ran this test with < 2GB heap or so. After this change it wouldn't use the 2GB limit, it would just allow growing to it (but the tests don't exercise growing to it as that's not relevant here).

[rstoyanchev]

I suppose the higher one needs to make contentCacheLimit for outliers, the less ideal it is as for use as an initial size for the average request body size.

The change seems reasonable to me, treating contentCacheLimit mostly as an upper boundary, but otherwise using the same default for an initial size. Or we could add one more constructor that takes defaultInitialSize in addition.

I would like more opinions from other team members.

[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.

[ryanrupp]

@bclozel

1. 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
2. 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.

[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.

[lonre]

Hi @bclozel

can we port f83c609 to branch 5.3.x?

[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.

[lonre]

@bclozel Yes, according to the jfr

ContentCachingRequestWrapper seems allocating much memory.

[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.

[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.

[bclozel]

@kilink what do you think about this change?

[kilink]

@kilink what do you think about this change?

@bclozel That looks good to me. Thanks!