commons-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niall Pemberton (JIRA)" <j...@apache.org>
Subject [jira] Updated: (BEANUTILS-291) Circular Reference on WeakHashMap
Date Fri, 14 Mar 2008 04:31:24 GMT

     [ 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 dev@commons 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)
> org.jboss.web.tomcat.service.WebAppClassLoader@16334564
> !--- sun.reflect.DelegatingClassLoader@27651708
> !--- !--- class sun.reflect.GeneratedConstructorAccessor38
> !--- !--- !--- [Ljava.lang.Object;@10800875
> !--- !--- !--- !--- java.util.Vector@838806
> !--- !--- !--- !--- !--- sun.reflect.DelegatingClassLoader@27651708
> !--- !--- !--- !--- !--- !--- class sun.reflect.GeneratedConstructorAccessor38
> !--- !--- !--- !--- !--- !--- !--- class java.lang.Class
> !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.converters.ClassConverter@22616909
> !--- !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.converters.ArrayConverter@18888821
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.converters.ConverterFacade@13619754
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- java.util.HashMap$Entry@32434103
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- [Ljava.util.HashMap$Entry;@28236766
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- java.util.HashMap@14997495
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.ConvertUtilsBean@2016953
> !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.BeanUtilsBean@30487951
> !--- !--- !--- !--- !--- !--- !--- !--- !-- -!--- !--- !--- !--- !--- !--- !---FieldReference
private java.lang.Object java.util.WeakHashMap$Entry.value=java.util.WeakHashMap$Entry@23775534
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.


Mime
View raw message