[
https://issues.apache.org/jira/browse/OFBIZ-5395?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13824250#comment-13824250
]
Jacques Le Roux commented on OFBIZ-5395:
----------------------------------------
Adrian,
>I don't understand. There are many places where OFBiz uses ThreadLocal
>variables.
Actually it's more a custom-class loader issue in the context of application
servers. This is well explained in the link from the "What it does (in less
than a minute)?" sentence above.
Note that:
* Only static ThreadLocal are concerned
* Only custom classes (like CompilerMatcher in ThreadLocal<CompilerMatcher> vs
Timestamp ThreadLocal<Timestamp>) are concerned. Because the JDK classess used
within ThreadLocal are not loaded by the webapp classloader which is [one of
the reasons you can have a memory
leaks|https://wiki.apache.org/tomcat/MemoryLeakProtection#customThreadLocal].
So "static ThreadLocal<RollbackOnlyCause>" and "static
ThreadLocal<List<RollbackOnlyCause>>" are also concerned because they are
custom classes. While "static ThreadLocal<List<Transaction>>" and "static
ThreadLocal<List<Exception>>" are not because JDK classes are loaded by the
default class loader ([Bootstrap in
Tomcat|http://tomcat.apache.org/tomcat-7.0-doc/class-loader-howto.html]).
* Note also that in all the memory leaks cases we have in OFBiz, any is of much
concern. Those are all tiny memory leaks. So OOTB it's not a problem. But in
the context of an application server, where you can deploy/undeploy web apps
many times, accumulating them might eventually be a problem.
>Another thing I don't understand: You start off pointing out how ThreadLocal
>variables can cause memory leaks, then you add a new ThreadLocal variable in
>your patch. Isn't that defeating your goal?
As explained above, it's mostly in the context of application servers where a
thread pool is used. So OOTB there are no massive issues. Of course tiny leaks
will arise OOTB since we also use a thread pool. But because OOTB we don't have
the deploy/undeploy cycles we are sort of imnune to those leaks. When we stop
OFBiz we also stop the thread pool and all the memory is recovered. That's why
I decided to go only with the Tomcat's JreMemoryLeakPreventionListener
solution. It's best of both worlds:
# protect OOTB usage from the memory leak caused when the first call to
sun.awt.AppContext.getAppContext() is triggered by a web appl. For more on the
weird awt leak, see page 14 of the link in the "History" word I put above.
Notably this sentence "Might be different if embedding Tomcat"!. When profiling
I saw many instance of it (sun.awt.AppContext), not huge (<100MB) but
noticeable (>10MB). Of course it's only one block for a loaded insance in our
case. But, I guess, in the context of an external application server, this
might be an harder issue.
# protect (or warn) in app server usage context from other kind of memory leaks
(exposed in the link from "Tomcat's JreMemoryLeakPreventionListener" sentence
above).
>Personally, I never cared for ThreadLocal variables. They appear hackish and
>they indicate a poor OO design.
They don't, but must be used carefully, notably when class loader issues may
arise.
Arguments:
* http://javapapers.com/core-java/threadlocal/#memoryleaks
*
http://javarevisited.blogspot.fr/2012/05/how-to-use-threadlocal-in-java-benefits.html
>It seems to me you are trying to solve a pattern matching problem. It might
>help to focus on that first.
Not at all, there are no pattern matching problems in OOTB code. Only possible
memory leaks, mostly related to ThreadLocal. Which led me to detect also the
awt one, and finally guided me to the Tomcat Memory Leak Protection. Still
those memory leaks are almost unnoticeable (tiny relative to the memory
available) in the OOTB context.
*In summary: Tomcat's JreMemoryLeakPreventionListener does not only make us
safe, or warn us about possible leaks, but it also allows us to continue to use
ThreadLocal serenely. Like Joshua Bloch, I now also believe ThreadLocals have
their advantages and are not intrinsically evil. I must say I doubted about
that last point while doing this research!*
> Introduce Tomcat's JreMemoryLeakPreventionListener and why
> ----------------------------------------------------------
>
> Key: OFBIZ-5395
> URL: https://issues.apache.org/jira/browse/OFBIZ-5395
> Project: OFBiz
> Issue Type: Improvement
> Components: framework
> Affects Versions: SVN trunk
> Reporter: Jacques Le Roux
> Assignee: Jacques Le Roux
> Labels: JreMemoryLeakPreventionListener, ThreadLocal, leak
> Attachments: OFBIZ-5395-CompilerMatcher.patch
>
>
> After reading few articles on possible memory leaks when using ThreadLocal
> variable with custom classes in application server context where a thread
> pool is used, I checked OFBiz code. There is only 2 custom classes
> concerned: CompilerMatcher and RollbackOnlyCause (JDK classes are not
> concerned by ThreadLocal memory leaks).
> First I must tell, that the memory leak problem is more clearly described in
> those articles when you use an external Application Server (like Tomcat) and
> you deploy/undeploy applications. It seems there are no major issues when you
> use OFBiz OOTB (with Tomcat embedded). Nevertheless, it's a concern by and
> large.
> I have not investigated RollbackOnlyCause, only CompilerMatcher. But, after
> some profiling, I believe both should only generate small amouts of memory
> leaks, almost not noticeable even after several deploy/undeploy cycles.
> Nevertheless I tried to find a good way to get rid of CompilerMatcher
> possible leaks. I thought about 3 ways:
> # *Reverts [CompilerMatcher related
> changes|http://svn.apache.org/viewvc?view=revision&revision=1075322]* done
> for OFBIZ-4107 (introduction of CompilerMatcher) by using
> Perl5Compiler.READ_ONLY_MASK which guarantees thread safety
> ** Pros: no need to introduce ThreadLocal
> ** Cons: performance, local Perl5 variables creation removes the
> patterns-compiled-cache CompilerMatcher introduced (note: [I found the origin
> of CompilerMatcher class
> here|http://mail-archives.apache.org/mod_mbox/jmeter-user/200212.mbox/%[email protected]%3E])
> # *Uses ThreadLocal<CompilerMatcher> local variables* instead of private
> static members
> ** Pros: no need to worry about thread safety
> ** Cons: performance, local ThreadLocal local variables creation removes the
> patterns-compiled-cache ThreadLocal<CompilerMatcher> offers when used as a
> private static member
> # *Uses ThreadLocal<CompilerMatcher> local variables* instead of private
> static members
> ** Pros: no need to worry about thread safety
> ** Cons: performance, local ThreadLocal local variables creation removes the
> patterns-compiled-cache ThreadLocal<CompilerMatcher> gives when used as a
> private static member
> # *Introduces [Tomcat's
> JreMemoryLeakPreventionListener|http://wiki.apache.org/tomcat/MemoryLeakProtection].*
> [What it does (in less than a
> minute)?|http://stackoverflow.com/questions/14882794/what-does-tomcats-threadlocalleakpreventionlistener-do-exactly]
> [Why JreMemoryLeakPreventionListener was
> created?|http://www.tomcatexpert.com/blog/2010/04/06/tomcats-new-memory-leak-prevention-and-detection]
> [History (29 pages
> presentation).|http://people.apache.org/~markt/presentations/2010-11-04-Memory-Leaks-60mins.pdf]
> How it does it? [Read
> code!|http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/core/JreMemoryLeakPreventionListener.java?view=annotate]
> ** Pros:
> *** no changes related to CompilerMatcher, performance enhancement the cache
> introduces kept
> *** prevents memory leaks when an external Application Server is used or at
> least warn about them
> ** Cons: none, this should had any noticeable effects when OFBiz is used OOTB
> (Tomcat embedded)
> So of course I decided to go with the JreMemoryLeakPreventionListener
> solution. Another reason for that is that when I profiled OFBiz trunk using
> the demo site I found that we were having a large bloc of memory retained by
> [sun.awt.AppContext|http://www.docjar.com/docs/api/sun/awt/AppContext.html].
> I think we should not have such a thing, the web truk demo does not use AWT
> at all! Fortunately jreMemoryLeakPreventionListener.setAppContextProtection
> prevents this, even if I have still no ideas from where this comes.
> I'm also considering to replace the current uses of java.util.regex.Pattern
> by CompilerMatcher in cases of a static pattern is used. Then the
> CompilerMatcher cache makes sense.
> Some interesting references I noted while analysing this issue:
> * [Oro is 6 times faster than regular Java
> regex|http://www.tusker.org/regex/regex_benchmark.html]. So, with its cache,
> CompilerMatcher is more than an interesting alternative to regular Java regex.
> * Java regex Javadoc:
> [Compiler|https://docs.oracle.com/javase/6/docs/api/java/util/regex/Pattern.html],
>
> [Matcher|https://docs.oracle.com/javase/6/docs/api/java/util/regex/Matcher.html]
> * Oro Javadoc:
> [Compiler|https://jakarta.apache.org/oro/api/org/apache/oro/text/regex/Perl5Compiler.html],
>
> [Matcher|https://jakarta.apache.org/oro/api/org/apache/oro/text/regex/Perl5Matcher.html]
--
This message was sent by Atlassian JIRA
(v6.1#6144)
