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).
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
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.
> 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.
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.
> 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.
> 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.
> - 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?
(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 ...
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
Another Thanks goes to Christian Lorenz for helping me understand the Tomcat sources and integrate the changes.
Any chances that this patch might make it into the 6.0.x line?
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.
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
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.
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.
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.
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.
See comment above.
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.
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.
Moving to 6.0.x as this feature has been present in 7.0.x for some time.
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.
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.