[
https://issues.apache.org/jira/browse/BEANUTILS-291?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Niall Pemberton updated BEANUTILS-291:
--------------------------------------
Attachment: BEANUTILS-291-FixMemoryLeaks-v1.patch
Clebert,
My apologies that its taken so long to get round to looking at your patches.
The memory leak test case, links to your wiki on Weak/SoftReferences and JBoss
Profiler were all very useful - thanks.
I have just commited a MemoryLeakTestCase - based on the one you supplied:
http://svn.apache.org/viewvc?view=rev&revision=636989
...but with additional tests because, unfortunately, there are more scenarios
within BeanUtils for memory leaks. I believe the following is the complete list
(including those you're know):
- PropertyUtilsBean, descriptorsCache, FastHashMap<Class, PropertyDescriptor[]>
- PropertyUtilsBean, mappedDescriptorsCache, FastHashMap<Class,
FastHashMap<String, MappedPropertyDescriptor>>
- MethodUtils, cache, WeakHashMap<MethodDescriptor, Method>
- WrapDynaClass, dynaClasses, Map<Class, WrapDynaClass>
- ConvertUtilsBean, converters, HashMap<Class, Converter>
- LocaleConvertUtilsBean, mapConverters, FastHashMap<Locale,
FastHashMap<Class, LocaleConverter>>
1) PropertyUtilsBean
--------------------
- descriptorsCache: FastHashMap<Class, PropertyDescriptor[]>
- mappedDescriptorsCache: FastHashMap<Class, FastHashMap<String,
MappedPropertyDescriptor>>
I looked at the changes you propose for PropertyUtilsBean and I don't believe
its necessary in this case to use your ProxyHashMap - which is effectively a
"cache of caches" keyed by ClassLoader. We already have the equivalent of this
called ContextClassLoaderLocal[1] which stores an instance of BeanUtilsBean for
each ClassLoader - and each BeanUtilsBean has a PropertyUtilsBean instance.
So looks to me like we simply need to change the descriptorsCache and
mappedDescriptorsCache to be WeakHashMap instead of FastHashMap. I'm not even
sure why those are FastHashMap because they have "fast" set to true and in
"fast" mode FastHashMap operates as a HashMap, with no synchronization.
I tried this out using your test case - it seems to resolve the memory leak for
descriptorsCache, but not mappedDescriptorsCache. From what I can tell there
are two additional problems with mappedDescriptorsCache:
- It uses MethodUtils which also has a cache that can leak - ignore that for
now I'll come to that below.
- MappedPropertyDescriptor(MPD) is a custom BeanUtils implementation of
PropertyDescriptor that has one Class and two Method instance variables.
Changing these to SoftReference seems to resolve the issue.
[1]
http://svn.apache.org/repos/asf/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ContextClassLoaderLocal.java
2) MethodUtils
--------------
cache: WeakHashMap<MethodDescriptor, Method>
The cache in MethodUtils is a static WeakHashMap instance and the Method value
stores seems to prevent gc. Putting the Method in a WeakReference seems to
resolve this. Perhaps it should be a SoftReference, but the only danger seems
to be that it will get re-created more often.
3) WrapDynaClass
----------------
dynaClasses: Map<Class, WrapDynaClass>
I had already done some work to move the static dynaClasses cache into a "by
ClassLoader" cache (using ContextClassLoaderLocal). Unfortunately its a hack
since it was "protected" visibility and I'm avoiding breaking binary
compatibility with the previous release. Anyway, your test showed that the
problem still wasn't resolved and the issue seems to be the Class instance
variable WrapDynaClass has. Moving this into a SoftReference seems to resolve
it.
4) ConvertUtilsBean
-------------------
converters: HashMap<Class, Converter>
There are two issues here - the cache and the fact that AbstractConverter
(which many of the Converters stored in the cache derive from) has a reference
to Class. Changing the HashMap to a WeakHashMap and putting the
AbstractConverter's Class reference into a SoftReference seems to resolve this.
However I think it would be best to remove the Class reference from
AbstractConverter all together, since if that reference is garbage collected
then the Converters will fail, unless some magic is done to re-create it.
5) LocaleConvertUtilsBean
-------------------------
mapConverters: FastHashMap<Locale, FastHashMap<Class, LocaleConverter>>
I believe this should have been straight forward to fix, simply replacing
FastHashMap<Class, LocaleConverter> with WeakHashMap<Class, LocaleConverter>.
However the FastHashMap is exposed in the API so changing to WeakHashMap breaks
compatibility. So I've tried creating a WeakFastHashMap, which is a nasty hack
but seems to work.
I'm attaching a patch to this ticket with some proposed changes - although it
uses SoftReference in AbstractConverter which I probably won't do as I said
above. These changes all seem to make the memory leak tests pass using JDK
1.5.0_07. However I tried running the tests using JDK 1.4.2_12 and JDK 1.3.1_18
and three out of the six memory leak tests fail (both PropertyUtilsBean tests
and the WrapDynaClass test) - so not sure why that is.
I'm hoping that you still have time and interest in this, despite how long its
taken for me to get round to doing anything. Any feedback on my modified
version of your tests and proposed changes would be very welcome.
Niall
> Circular Reference on WeakHashMap
> ---------------------------------
>
> Key: BEANUTILS-291
> URL: https://issues.apache.org/jira/browse/BEANUTILS-291
> Project: Commons BeanUtils
> Issue Type: Bug
> Components: Bean / Property Utils
> Affects Versions: 1.8.0-BETA
> Reporter: Niall Pemberton
> Fix For: 1.8.0
>
> Attachments: BEANUTILS-291-FixMemoryLeaks-v1.patch,
> memoryLeakTests-new.zip, PropertyUtilsBean.java
>
>
> Clebert Suconic wrote on the [EMAIL PROTECTED] list ....
> (see http://tinyurl.com/2a9gan)
> I have been investigating WeakHashMaps on BeanUtils 1.8 as part of a
> investigation on this:
> http://jira.jboss.com/jira/browse/JBAS-2299
> (Which is not actually an issue with JBAS, but an issue when using BeanUtils
> as part of the classPath).
> There is a circular reference on the WeakHashMap, The WeakHashMap will have
> the ClassLoader as the key, and it will have a reference back to the Key from
> one of the Reflection objects. This doesn't work! (Please.. no discussions
> about this point.. if you don't believe me, do some testing with simple stuff
> before discussing this and come back to me only after that)
> [EMAIL PROTECTED]
> !--- [EMAIL PROTECTED]
> !--- !--- class sun.reflect.GeneratedConstructorAccessor38
> !--- !--- !--- [Ljava.lang.Object;@10800875
> !--- !--- !--- !--- [EMAIL PROTECTED]
> !--- !--- !--- !--- !--- [EMAIL PROTECTED]
> !--- !--- !--- !--- !--- !--- class sun.reflect.GeneratedConstructorAccessor38
> !--- !--- !--- !--- !--- !--- !--- class java.lang.Class
> !--- !--- !--- !--- !--- !--- !--- !--- [EMAIL PROTECTED]
> !--- !--- !--- !--- !--- !--- !--- !--- !--- [EMAIL PROTECTED]
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- [EMAIL PROTECTED]
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- [EMAIL PROTECTED]
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !---
> [Ljava.util.HashMap$Entry;@28236766
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- [EMAIL
> PROTECTED]
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- [EMAIL
> PROTECTED]
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !---
> [EMAIL PROTECTED]
> !--- !--- !--- !--- !--- !--- !--- !--- !-- -!--- !--- !--- !--- !--- !---
> !---FieldReference private java.lang.Object [EMAIL PROTECTED] Detail
> I don't know if I'm preaching to the choir, but just in case this is new
> information to someone... you should aways keep Reflection referenced as
> SoftReferences (if you really have to). Reflection is aways a new object so a
> WeakReference is too weak.
> On JBossSerialization I have solved this using an interesting way. I called
> it PersistentReference. I'm using SoftReferences, and keeping the information
> to recreate it case the SoftReference is cleared:
> http://fisheye.jboss.org/browse/JBoss/jboss-serialization/src/org/jboss/serial/references/PersistentReference.java?r=1.3
> And also... you guys should write a testcase to validate if the Caching is
> being cleared. (I don't know if you have one).
> http://anonsvn.jboss.org/repos/jbossserialization/trunk/tests/org/jboss/serial/memory/MemoryLeakTestCase.java
> You don't need to use the jboss-profiler API for this.. just create a
> WeakReference to a new ClassLoader, and validate if it was released at the
> end after some exercizing some code on this caching. You will
> probably need to fill your memory almost to 100% on the test as SoftReference
> are only gone when the memory is low.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
