Skip to content

Commit

Permalink
Optimize ContentCachingRequestWrapper allocation
Browse files Browse the repository at this point in the history
This commit builds on top of changes made in gh-29775 and gh-31737.
Before this change, we would allocate several byte arrays even in cases
of known request size. This could decrease performance when getting the
cached content as it requires merging several arrays and data is not
colocated in memory.

This change ensures that we create a `FastByteArrayOutputStream`
instance with the known request size so that the first allocated segment
can contain the entire content.
If the request size is not know, we will default back on the default
allocation size for the `FastByteArrayOutputStream`.

Closes gh-31834
  • Loading branch information
bclozel committed Dec 13, 2023
1 parent a01c6d5 commit 0970b1d
Showing 1 changed file with 5 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
*/
public class ContentCachingRequestWrapper extends HttpServletRequestWrapper {

private final FastByteArrayOutputStream cachedContent = new FastByteArrayOutputStream();
private FastByteArrayOutputStream cachedContent;

@Nullable
private final Integer contentCacheLimit;
Expand All @@ -74,6 +74,8 @@ public class ContentCachingRequestWrapper extends HttpServletRequestWrapper {
*/
public ContentCachingRequestWrapper(HttpServletRequest request) {
super(request);
int contentLength = request.getContentLength();
this.cachedContent = (contentLength > 0) ? new FastByteArrayOutputStream(contentLength) : new FastByteArrayOutputStream();
this.contentCacheLimit = null;
}

Expand All @@ -86,6 +88,8 @@ public ContentCachingRequestWrapper(HttpServletRequest request) {
*/
public ContentCachingRequestWrapper(HttpServletRequest request, int contentCacheLimit) {
super(request);
int contentLength = request.getContentLength();
this.cachedContent = (contentLength > 0) ? new FastByteArrayOutputStream(contentLength) : new FastByteArrayOutputStream();

This comment has been minimized.

Copy link
@palacsint

palacsint Jun 7, 2024

Shouldn't FastByteArrayOutputStream(contentLength) be constrained by contentCacheLimit? For example, if it receives a request with 50MB of content but the contentCacheLimit is only 1MB, it could result in unintended memory usage.

This comment has been minimized.

Copy link
@bclozel

bclozel Jun 7, 2024

Author Member

Thanks for noticing, I will take care of this in #32987

this.contentCacheLimit = contentCacheLimit;
}

Expand Down

0 comments on commit 0970b1d

Please sign in to comment.