Subject: Questions on Jasper's BodyContentImpl, bug #43925, and proposed solution

[Scott Severtson <ss...@digitalmeasures.com>]

All,

We've noticed that our Tomcat 6.0.37 instances have increasing overhead 
the longer they stay up. We've captured some heap dumps, expecting to 
find memory leaks within our code, but found something different instead.

Jasper's BodyContentImpl is actually holding most of the non-collectible 
garbage, in the internal char[] buffer. I've checked trunk of Tomcat 6, 
7 and 8, and see that the same implementation is still used, so this 
issue applies to all currently supported versions.

This issue appears to be well known:
* 
http://stackoverflow.com/questions/10421908/is-there-a-fix-for-bodycontentimpl-jsp-tag-memory-leak-using-tomcat
* https://issues.apache.org/bugzilla/show_bug.cgi?id=43925

Specifically what we see happening is due to large pages being rendered 
in JSPs - sometimes in the multi-megabyte range, especially for internal 
tools. Apparently, some of the tags we use require buffering the output, 
versus sending it directly to the Writer.

Now, we certainly can simply set LIMIT_BUFFER and forget about it (throw 
away any buffers larger than 8k), but this isn't optimal for our 
application. For about 75% of our requests, ~8k is the baseline page 
size, with many larger than that. So, depending how the data size, we 
could be reallocating multiple times, only to throw away the buffer at 
the end.

We'd like to propose a different behavior, and are wondering what others 
think about it. If it sounds like a good idea, we're certainly willing 
to submit patches to implement it.

--Proposal--
Instead of a hard-coded DEFAULT_TAG_BUFFER_SIZE of 8k, make it 
configurable. In addition, if LIMIT_BUFFER is set, provide another 
configurable setting called something like MAX_TAG_BUFFER_SIZE (if not 
set, defaults to DEFAULT_TAG_BUFFER_SIZE). This would give applications 
a more predictable *range* of memory allocated for tag buffers, while 
decreasing the frequency of reallocation.
--/Proposal--

We'd probably set DEFAULT_TAG_BUFFER_SIZE to 16k, and set 
MAX_TAG_BUFFER_SIZE to either 64k or 128k, based on real-world 
performance testing.


One other, slightly less defined question - is there a reason we're 
managing char arrays ourselves, instead of using something like Java's 
StringBuilder or Javalution's TextBuilder? StringBuilder would reduce 
code maintenance, while something like TextBuilder would also reduce 
array allocation.

We'd love to hear your thoughts.

Thanks,
Scott Severtson
Lead Platform Architect
Digital Measures

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Subject: Questions on Jasper's BodyContentImpl, bug #43925, and proposed solution

[Jeremy Boynes <jb...@apache.org>]

On Nov 1, 2013, at 6:23 AM, Scott Severtson <ss...@digitalmeasures.com> wrote:

> All,
> 
> We've noticed that our Tomcat 6.0.37 instances have increasing overhead the longer they stay up. We've captured some heap dumps, expecting to find memory leaks within our code, but found something different instead.
> 
> Jasper's BodyContentImpl is actually holding most of the non-collectible garbage, in the internal char[] buffer. I've checked trunk of Tomcat 6, 7 and 8, and see that the same implementation is still used, so this issue applies to all currently supported versions.
> 
> This issue appears to be well known:
> * http://stackoverflow.com/questions/10421908/is-there-a-fix-for-bodycontentimpl-jsp-tag-memory-leak-using-tomcat
> * https://issues.apache.org/bugzilla/show_bug.cgi?id=43925
> 
> Specifically what we see happening is due to large pages being rendered in JSPs - sometimes in the multi-megabyte range, especially for internal tools. Apparently, some of the tags we use require buffering the output, versus sending it directly to the Writer.
> 
> Now, we certainly can simply set LIMIT_BUFFER and forget about it (throw away any buffers larger than 8k), but this isn't optimal for our application. For about 75% of our requests, ~8k is the baseline page size, with many larger than that. So, depending how the data size, we could be reallocating multiple times, only to throw away the buffer at the end.
> 
> We'd like to propose a different behavior, and are wondering what others think about it. If it sounds like a good idea, we're certainly willing to submit patches to implement it.
> 
> --Proposal--
> Instead of a hard-coded DEFAULT_TAG_BUFFER_SIZE of 8k, make it configurable. In addition, if LIMIT_BUFFER is set, provide another configurable setting called something like MAX_TAG_BUFFER_SIZE (if not set, defaults to DEFAULT_TAG_BUFFER_SIZE). This would give applications a more predictable *range* of memory allocated for tag buffers, while decreasing the frequency of reallocation.
> --/Proposal--

Allowing for a cap on growth and throwing an IOException when it is hit seems like a reasonable precaution to prevent more drastic OOM errors (I've been bitten by this too).

Using a threshold rather than simple boolean to control recycling could be helpful. It could easily mimic the current functionality albeit at the cost of backward compatibility. However, I would expect see allocations gradually creep up to this limit across the board so it seems more predictable just to raise the default to that value.

> We'd probably set DEFAULT_TAG_BUFFER_SIZE to 16k, and set MAX_TAG_BUFFER_SIZE to either 64k or 128k, based on real-world performance testing.

DEFAULT_TAG_BUFFER_SIZE is only 512 bytes and, AIUI, is set low-ish just to cover the common case where fragments generate small amounts of output. I don't think we should change the defaults for these.

> One other, slightly less defined question - is there a reason we're managing char arrays ourselves, instead of using something like Java's StringBuilder or Javalution's TextBuilder? StringBuilder would reduce code maintenance, while something like TextBuilder would also reduce array allocation.

Patches are always welcome.

Cheers
Jeremy