Bug 31789

Summary: Memory leak in ELEvaluator
Product: Taglibs Reporter: Daryl Beattie <darylb>
Component: Standard TaglibAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: major CC: crv, darylb, knut
Priority: P3    
Version: 1.0.6   
Target Milestone: ---   
Hardware: Other   
OS: other   
Bug Depends on:    
Bug Blocks: 43544    
Attachments: The String cache size of my application after ~ 35 mins.
The String cache hit rate of my application after ~ 35 mins.
The String cache calls by my application after ~ 35 mins.
eval() calls non-expressions vs. expressions ratio in custom tags
eval() calls non-expressions vs. expressions ratio in JSTL tags
String cache size with indexOf("${") bypass enabled.
String cache calls with indexOf("${") bypass enabled (in custom tags).
String cache hit-rate with indexOf("${") bypass enabled (in custom tags).
SPI like fix for this issue, 17700 and 32311
Updated version of the caching factory patch
Patch implementing Kris's mBypassCache suggestion
Slight modification of Bjorn's patch
Implementation of LRU cacheing strategy

Description Daryl Beattie 2004-10-19 21:28:08 UTC
This is actually a bug for version 1.0.6 (but I couldn't select that version
because it wasn't listed).

WHAT IS IT?:
     All evaluated expressions are added to the ELEvaluator's
sCachedExpressionStrings map and never removed. This causes a memory leak which
can eventually cause running programs to crash due to an OutOfMemoryError.

WHERE IS IT?:
     org.apache.taglibs.standard.lang.jstl.ELEvaluator.parseExpressionString(String)

TO REPRODUCE:
     Write a simple page that uses a JSTL tag, give it a different
dynamically-generated expression as an attribute value and refresh the page
several times. Eventually you will see the memory climb (through profiling,
system logs or by your process exiting with an OutOfMemoryError).

A POSSIBLE FIX:
     A possible fix would be to use a last-recently-used cache (such as Java
1.4's LinkedHashMap) to ensure that items do not remain in this cache indefinitely.
Comment 1 Felipe Leme 2004-10-20 20:40:38 UTC
Just for the record, there is a thread going on the devs list about how to solve
this issue:

http://nagoya.apache.org/eyebrowse/BrowseList?listName=taglibs-dev@jakarta.apache.org&by=thread&from=903348

BTW, I have some suggesions to make, but will do it from home (as my email
client is not configured here at work to send the proper From: id). Meanwhile, I
would suggest Daryl to use this bug for uploading anything.

-- Felipe
Comment 2 Daryl Beattie 2004-10-20 21:18:21 UTC
Created attachment 13161 [details]
The String cache size of my application after ~ 35 mins.
Comment 3 Daryl Beattie 2004-10-20 21:18:49 UTC
Created attachment 13162 [details]
The String cache hit rate of my application after ~ 35 mins.
Comment 4 Daryl Beattie 2004-10-20 21:19:05 UTC
Created attachment 13163 [details]
The String cache calls by my application after ~ 35 mins.
Comment 5 Daryl Beattie 2004-10-22 15:37:38 UTC
Created attachment 13189 [details]
eval() calls non-expressions vs. expressions ratio in custom tags
Comment 6 Daryl Beattie 2004-10-22 15:37:59 UTC
Created attachment 13190 [details]
eval() calls non-expressions vs. expressions ratio in JSTL tags
Comment 7 Daryl Beattie 2004-10-22 15:39:47 UTC
Created attachment 13191 [details]
String cache size with indexOf("${") bypass enabled.
Comment 8 Daryl Beattie 2004-10-22 15:40:21 UTC
Created attachment 13192 [details]
String cache calls with indexOf("${") bypass enabled (in custom tags).
Comment 9 Daryl Beattie 2004-10-22 15:40:54 UTC
Created attachment 13193 [details]
String cache hit-rate with indexOf("${") bypass enabled (in custom tags).
Comment 10 Dhiru Pandey 2005-02-23 19:35:08 UTC
*** Bug 30136 has been marked as a duplicate of this bug. ***
Comment 11 Henri Yandell 2007-02-07 17:32:21 UTC
Created attachment 19542 [details]
SPI like fix for this issue, 17700 and 32311

Patch attached which creates a pluggable system via Java system properties to
allow for different types of caches to resolve the problems expressed in this
issue, and in #32311 and #17700. 

The Java system property configuration seems weak, but I'm not sure what you
could replace it with without changing a bunch of code.

Completely untested :) Will aim to test it next week.
Comment 12 Henri Yandell 2007-02-07 17:34:18 UTC
There are other locations that might require the caching strategy.
tag.common.fmt would seem to have a few.
Comment 13 Henri Yandell 2007-08-24 07:59:50 UTC
Created attachment 20701 [details]
Updated version of the caching factory patch

This is a tested version of the original patch, with a few bugs fixed. Things
should behave the same as before by default, but you can either turn on caching
(forever) or turn off caching (null) to ease some of these issues.
Alternatively you could write your own cache.

For DateFormat for example, speed improves 30% if you turn caching of
DateFormat objects on.

One issue is that for ELEvaluator parsing, I've removed the synchronization
that was being done. I need to change the patch so that that is still there.
Comment 14 Henri Yandell 2007-08-24 08:54:19 UTC
Scratch the synchronization comment. Had been a bit since I'd looked at the
code. The ForeverCache maintains the synchronization, so it's at a lower level.

The patch has a bug in the Resources class where it still uses format(String,
Object[]) and hasn't had the 'res' deleted to make it use format(Object[]). Even
with that fix, I can't make a fmt:message faster.
Comment 15 Bjorn Townsend 2007-09-21 15:06:08 UTC
Created attachment 20866 [details]
Patch implementing Kris's mBypassCache suggestion

Here's a patch that implement's Kris's mBypassCache.
Comment 16 Henri Yandell 2007-09-22 11:39:44 UTC
Also reported to EL as:

http://issues.apache.org/jira/browse/EL-1
Comment 17 Henri Yandell 2007-09-22 12:32:22 UTC
Created attachment 20868 [details]
Slight modification of Bjorn's patch

I've attached a slight modification to Bjorn's patch - adding a note to the
javadoc and restricting the change to the ELEvaluator class.
Comment 18 Bjorn Townsend 2007-09-26 12:59:29 UTC
Created attachment 20887 [details]
Implementation of LRU cacheing strategy

After doing a ton of research, I found that someone had actually branched 1.0.6
and implemented a LRU-based cacheing strategy, with the size of the cache
definable by the user in a context initialization parameter. The implementation
uses a set of classes borrowed from Commons Collections, but does not introduce
Collections as a dependancy. I found this code in the following branch:

http://svn.apache.org/repos/asf/jakarta/taglibs/proper/standard/branches/STANDARD_1_0_BRANCH


The default cache size is 100 entries. It'd probably be wise to experiment with
this value in one's own environment before settling on an optimum size.

After a conversation with Henri on the taglibs dev list, I've gone ahead and
adapted the patch to work with 1.1.  I've already run it through the taglibs
unit test suite -- all tests passed. This patch also fixes the issue whereby
cacheing could not be turned off.
Comment 19 Bjorn Townsend 2007-09-26 13:03:37 UTC
And for the sake of completeness, here's a link to the new mailing list
discussion on this bug:

http://mail-archives.apache.org/mod_mbox/jakarta-taglibs-dev/200709.mbox/browser
Comment 20 Henri Yandell 2007-09-29 22:55:14 UTC
The 1.0.6 fix has been applied to the 1.1 branch.