Bug 48358 - JSP-unloading reloaded
Summary: JSP-unloading reloaded
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Jasper (show other bugs)
Version: unspecified
Hardware: All All
: P2 enhancement (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-09 06:58 UTC by Isabel Drost
Modified: 2017-03-10 13:06 UTC (History)
0 users



Attachments
Patch including tests that fixed the problem for us. (15.20 KB, patch)
2009-12-09 06:58 UTC, Isabel Drost
Details | Diff
Patch including tests that adds support for selectively unloading jsps. (24.23 KB, patch)
2009-12-15 01:53 UTC, Isabel Drost
Details | Diff
Trivial fix for NPE problem for problem clarification (811 bytes, patch)
2011-01-28 10:18 UTC, Isabel Drost
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Isabel Drost 2009-12-09 06:58:37 UTC
Created attachment 24684 [details]
Patch including tests that fixed the problem for us.

Currently Tomcat does not support unloading JSPs. When constantly changing and reloading JSP files (especially during development time) this causes the JVM to run out of memory.

The patch fixes this problem by tracking the last time a JSP page was requested. Objects are destroyed if a configurable number of JSPs is live, starting with the "oldest" ones. This behaviour is deactivated by default. It must be configured explicitly.

The patch comes with tests that check the added functionality. To make testing easier I added easymock as dependency to the classpath.

I'd appreciate any feedback on the code changes - suggestions for improvement, potential problems with the code etc.

This work was done in collaboration with some of my colleagues at work. I will forward a link to this bug entry to those involved so they can provide more information on the background of the patch if needed.

On a side note: Just in case you might like to include the patch - I couldn't find a "patch intended for inclusion" check box (like the one in jira) in bugzilla - thus stating explicitly: License to ASF granted for inclusion in ASF works (as per the Apache License ยง5).
Comment 1 Peter Rossbach 2009-12-10 07:28:36 UTC
Hi Isabel,

feature looks fine for me :-) Many Thanks.

But some comments...

- Patch missing message  jsp.warning.maxLoadedJsps at java/org/apache/jasper/resources/LocalStrings.properties

- The oldest JSP search seams not cheap. Some CMS-Sites have more 30000 active JSP's!

- Have you tested your JSP unloading with heavy load?

Peter
Comment 2 Mark Thomas 2009-12-10 09:22:47 UTC
As a point of clarification, if a JSP is changed then the old implementation should be unloaded before the new implementation is loaded. Therefore, lots of changes to a small number of pages shouldn't cause an issue. If such a scenario does cause an issue then that is a bug and I would ask that you please open a separate issue.

This enhancement appears to be addressing the use case where an application consists of many thousands of pages that are rarely used so, in an effort to conserve resources, pages that have not been accessed for a while are unloaded. I can see how this could be useful in development, particularly if memory is tight on an individual developer's machine. In production, I think it would be easier (and give better performance) to spend few hundred dollars on some extra memory for the server.

I also share Peter's concerns about the cost of the oldest JSP search and think that this part of the patch needs to be revisited.
Comment 3 Isabel Drost 2009-12-10 12:27:05 UTC
> I also share Peter's concerns about the cost of the oldest JSP search and
> think that this part of the patch needs to be revisited

I agree - will look into that ASAP - Thanks pointing out the problem.
Comment 4 Isabel Drost 2009-12-15 01:53:57 UTC
Created attachment 24707 [details]
Patch including tests that adds support for selectively unloading jsps.

The patch includes changes to the way of identifying the jsp(s) to destroy: If unloading is activated, the JspRuntimeContext tracks the relative "age" - in terms of last execution time - of each jsp.

Age is tracked in a queue that supports additions, updates and removal of its nodes in constant time.

If unloading is activated this gets rid of the costly loop for identifying candidate jsps. It should incur only slight overhead at jsp execution time.
Comment 5 Isabel Drost 2009-12-15 01:57:40 UTC
> Have you tested your JSP unloading with heavy load?

We ran initial smoke-tests against artificial setups as well as load tests against a "real-world" web application with user requests extracted from log files on our test systems. So far everything looks good. Further tests are currently being run. Will report the results as soon as they are available.
Comment 6 Isabel Drost 2009-12-15 02:03:43 UTC
> As a point of clarification, if a JSP is changed then the old implementation
> should be unloaded before the new implementation is loaded. Therefore, lots of
> changes to a small number of pages shouldn't cause an issue.

You are right: 1) My explanation was wrong in this respect, 2) re-loading existing jsps this doesn't cause an issue.
Comment 7 Isabel Drost 2010-01-14 02:17:49 UTC
> - Have you tested your JSP unloading with heavy load?

The refined patch was tested with some 400 requests per second (up to 700 max) - all looked fine.

Mark, do you have any comments on the refined version of the patch? Anything in particular I should clarify?
Comment 8 Marko Kitzing 2010-03-09 15:19:44 UTC
(In reply to comment #1)

> - Have you tested your JSP unloading with heavy load?
> 
> Peter

We're using the patched version of tomcat for all *.lecker.de and *.wunderweib.de domains, i.e. http://tinakochen.lecker.de.
We're talking about appr. 20Mio PI/month over all portals. Might not be heavy load, but it's pretty high load.
The patch made operation of the portals much easier and reduced the number of forced restarts of tomcat to one scheduled restart every night.
Prior the patch we were heading towards 10-20 restarts of each of the two load balanced tomcats each day, resulting in temporarily unavailability of the whole site from time to time, because both tomcats have been automaticaly restarted in parallel ...
Comment 9 Mark Thomas 2010-04-25 08:21:56 UTC
Thanks for the patch. It has been applied to 7.0.x and will be included in 7.0.0.

I had to make a few changes:
- correct indentation
- add documentation
- add AL2 headers
Comment 10 Isabel Drost 2010-04-28 10:55:51 UTC
Another Thanks goes to Christian Lorenz for helping me understand the Tomcat sources and integrate the changes.
Comment 11 Christian Brandel 2010-11-17 03:43:21 UTC
Any chances that this patch might make it into the 6.0.x line?
Comment 12 Rainer Jung 2010-11-18 06:01:15 UTC
Thanks for the reminder.

The patch has substantially changed since it has originally been applied to Tomcat 7. The updated patch for TC 6 is available at

http://people.apache.org/~rjung/patches/BZ48358-JSP_Unloading-TC6_20101118.patch

The patch itself is low risk because the feature is off by default. Committers will have to decide whether we want to keep the feature as a new feature in TC 7, or whether the problem addressed is important enough to warrant applying it to TC 6. I proposed it for backport.
Comment 13 Christian Brandel 2010-11-22 04:58:13 UTC
Excellent, Rainer!

Excuse my ignorance, if this is somehow obvious from the patch itself (this'll be my first patch): Against which version can this be applied? 6.0.29? 6.x trunk from SVN?

Thanks,
Chris
Comment 14 Rainer Jung 2010-11-22 09:18:24 UTC
As far as I remember it should apply cleanly to 6.0.29 and 6.0.x trunk (as of now). Try it and shout if it doesn't work.
Comment 15 Christian Brandel 2010-12-02 10:00:12 UTC
Tha patch applies cleanly to 6.0.29 from the src archive.
However it does not work for me the way I expected it and I am still facing PermGen Out Of Memory errors.

- The counters in the JspMonitor MBean for jspCount, jspReloadCount, jspUnloadCount, jspQueueLength were updated appropriately

- When running with -XX:+TraceClassUnloading I did not see any org.apache.jsp.* classes being unloaded until I shut down the Tomcat process

- Looking at a Heap Dump written at the time of the crash, I saw 337 instances of org.apache.jsp.xxx classes being held while I configured the maxLoadedJSPs to be 200

Are there any additional settings that have to be adapted? My current settings are:

Java 1.6 32bit on Windows 2003 Server with the following settings (memory parameters are set to small values to shorten the time until tomcat crashes)

-Xms256m -Xmx256m
-XX:MaxPermSize=48M
-XX:+UseConcMarkSweepGC
-XX:+CMSIncrementalMode
-XX:+CMSClassUnloadingEnabled
-XX:+CMSPermGenSweepingEnabled
-XX:+PrintGCDetails
-XX:+PrintGCTimeStamps
-XX:+HeapDumpOnOutOfMemoryError
-XX:HeapDumpPath=E:\logs\wcmInternet\ContentProxy1\tomcat
-Dorg.apache.jasper.compiler.Parser.STRICT_QUOTE_ESCAPING=false
-Dorg.apache.jasper.runtime.BodyContentImpl.LIMIT_BUFFER=true
-Dorg.apache.jasper.runtime.BodyContentImpl.USE_POOL=false
-XX:+TraceClassUnloading

Global web.xml:
    <servlet>
        <servlet-name>jsp</servlet-name>
        <servlet-class>org.apache.jasper.servlet.JspServlet</servlet-class>	
	<init-param>
            <param-name>fork</param-name>
            <param-value>false</param-value>
        </init-param>
        <init-param>
            <param-name>development</param-name>
            <param-value>true</param-value>
        </init-param>
		<init-param>
            <param-name>enablePooling</param-name>
            <param-value>false</param-value>
        </init-param>
        <init-param>
            <param-name>modificationTestInterval</param-name>
            <param-value>0</param-value>
        </init-param>
        <init-param>
            <param-name>genStringAsCharArray</param-name>
            <param-value>true</param-value>
        </init-param>
		<init-param>
            <param-name>xpoweredBy</param-name>
            <param-value>false</param-value>
        </init-param>
        <init-param>
            <param-name>maxLoadedJsps</param-name>
            <param-value>200</param-value>
        </init-param>		
        <load-on-startup>3</load-on-startup>
    </servlet>

Side note: If this is not the correct place to discuss this, I'm happy to continue it elsewhere.
Comment 16 Christian Brandel 2010-12-02 11:35:07 UTC
Update:

I ran a similar test against the examples webapp with maxLoadedJsps set to 10. Right before and after a manually triggered GC there were 43 instances of org.apache.jsp. classes (jsps and some tag instances) in the heap dump.
So it looks to me as if somehow the classes are not unloaded.

(In reply to comment #15)
> Tha patch applies cleanly to 6.0.29 from the src archive.
> However it does not work for me the way I expected it and I am still facing
> PermGen Out Of Memory errors.
> 
> - The counters in the JspMonitor MBean for jspCount, jspReloadCount,
> jspUnloadCount, jspQueueLength were updated appropriately
> 
> - When running with -XX:+TraceClassUnloading I did not see any org.apache.jsp.*
> classes being unloaded until I shut down the Tomcat process
> 
> - Looking at a Heap Dump written at the time of the crash, I saw 337 instances
> of org.apache.jsp.xxx classes being held while I configured the maxLoadedJSPs
> to be 200
> 
> Are there any additional settings that have to be adapted? My current settings
> are:
> 
> Java 1.6 32bit on Windows 2003 Server with the following settings (memory
> parameters are set to small values to shorten the time until tomcat crashes)
> 
> -Xms256m -Xmx256m
> -XX:MaxPermSize=48M
> -XX:+UseConcMarkSweepGC
> -XX:+CMSIncrementalMode
> -XX:+CMSClassUnloadingEnabled
> -XX:+CMSPermGenSweepingEnabled
> -XX:+PrintGCDetails
> -XX:+PrintGCTimeStamps
> -XX:+HeapDumpOnOutOfMemoryError
> -XX:HeapDumpPath=E:\logs\wcmInternet\ContentProxy1\tomcat
> -Dorg.apache.jasper.compiler.Parser.STRICT_QUOTE_ESCAPING=false
> -Dorg.apache.jasper.runtime.BodyContentImpl.LIMIT_BUFFER=true
> -Dorg.apache.jasper.runtime.BodyContentImpl.USE_POOL=false
> -XX:+TraceClassUnloading
> 
> Global web.xml:
>     <servlet>
>         <servlet-name>jsp</servlet-name>
>         <servlet-class>org.apache.jasper.servlet.JspServlet</servlet-class>    
>     <init-param>
>             <param-name>fork</param-name>
>             <param-value>false</param-value>
>         </init-param>
>         <init-param>
>             <param-name>development</param-name>
>             <param-value>true</param-value>
>         </init-param>
>         <init-param>
>             <param-name>enablePooling</param-name>
>             <param-value>false</param-value>
>         </init-param>
>         <init-param>
>             <param-name>modificationTestInterval</param-name>
>             <param-value>0</param-value>
>         </init-param>
>         <init-param>
>             <param-name>genStringAsCharArray</param-name>
>             <param-value>true</param-value>
>         </init-param>
>         <init-param>
>             <param-name>xpoweredBy</param-name>
>             <param-value>false</param-value>
>         </init-param>
>         <init-param>
>             <param-name>maxLoadedJsps</param-name>
>             <param-value>200</param-value>
>         </init-param>        
>         <load-on-startup>3</load-on-startup>
>     </servlet>
> 
> Side note: If this is not the correct place to discuss this, I'm happy to
> continue it elsewhere.
Comment 17 Isabel Drost 2011-01-28 10:18:07 UTC
Created attachment 26571 [details]
Trivial fix for NPE problem for problem clarification

After checking the changes made to the initial patch version I think I have found the main reason why unloading currently does not work as expected:

In org.apache.jasper.servlet.JspServlet in line 385 you call rctxt.addWrapper(...) which adds the jsp to the list of known jsps in the runtime context. However, for each wrapper the pointer to the unloadHandle is not updated before wrapper.service is called (same file, line 391).

In the org.apache.jasper.compilerJspRuntimeContext in "checkUnload" (which is triggered periodically) you iterate over the list of jsps, calling jsw.getUnloadHandle() in line 609. This method will return null, until wrapper.service has been called. As a result the unload loop crashes with an NPE in that case:

org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor 
processChildren
SCHWERWIEGEND: Exception invoking periodic operation:
java.lang.NullPointerException at org.apache.jasper.util.FastRemovalDequeue$Entry.access$700(FastRemovalDequeue.java:250)
         at org.apache.jasper.util.FastRemovalDequeue.remove(FastRemovalDequeue.java:173)
         at org.apache.jasper.compiler.JspRuntimeContext.checkUnload(JspRuntimeContext.java:609)
         at org.apache.jasper.servlet.JspServlet.periodicEvent(JspServlet.java:360)
         at org.apache.catalina.core.StandardWrapper.backgroundProcess(StandardWrapper.java:660)
         at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.processChildren(ContainerBase.java:1393)
         at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.processChildren(ContainerBase.java:1403)
         at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.processChildren(ContainerBase.java:1403)
         at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.processChildren(ContainerBase.java:1403)
         at org.apache.catalina.core.ContainerBase$ContainerBackgroundProcessor.run(ContainerBase.java:1382)
         at java.lang.Thread.run(Thread.java:619)


The attached patch fixed the problem for us - however neither does it solve the root cause of the issue, nor am I certain that it works in general (e.g. there is another call to addWrapper in TagFileProcessor). Patch is merely for clarification than for fixing the issue.
Comment 18 Isabel Drost 2011-01-28 10:19:26 UTC
See comment above.
Comment 19 Konstantin Kolinko 2011-02-14 11:38:25 UTC
One thought about limitations of this feature:
as far as I remember, and as mentioned in the Javadoc comment for java.lang.String#intern(), all String constants are placed by JVM into the same global cache.

Unloading the JSPs will not free those strings. Though if new versions of JSPs do not differ from the old ones in their text, there will be no noticeable consequences.

See also bug 50726.

(In reply to comment #15)
>         <init-param>
>             <param-name>genStringAsCharArray</param-name>
>             <param-value>true</param-value>
>         </init-param>

There is a typo above. The correct name of the above init-param name is genStrAsCharArray. See comments in conf/web.xml or the code of EmbeddedServletOptions class.
Comment 20 Christian Brandel 2011-04-26 10:22:35 UTC
Konstantin:

there is no typo.
At least for Tomcat Version 7.0.12 it's not getStrAsCharArray but genStringAsCharArray. The only occurrence of the string "getStrAsCharArray" I can find is in the localization files (LocalStrings.properties  etc.).

The class org.apache.jasper.EmbeddedServletOptions contains: 

String genCharArray = config.getInitParameter("genStringAsCharArray");

Comments in web.xml also refer to genStringAsCharArray.
Comment 21 Mark Thomas 2012-01-15 21:56:52 UTC
Moving to 6.0.x as this feature has been present in 7.0.x for some time.
Comment 22 trent.bartlem 2012-07-25 04:43:34 UTC
I'm not so sure this problem is restricted to Tomcat 6.

I encountered the same issues as Isabel Drost in comment #17 on a fresh Tomcat 7 install with jspIdleTimeout (and maxLoadedJsps) set.
Comment 23 Mark Thomas 2017-03-10 13:06:57 UTC
6.0.x is now end-of-line so I have not reviewed the 6.0.x code.

The 7.0.x code is working as expected. It has been checked via a manual test and a profiler (thanks again to YourKit) as well as by code inspection for the issues raised in the most recent comments.

I am, therefore, marking this issue as fixed.