openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Craig L Russell <Craig.Russ...@Sun.COM>
Subject Re: PCRegistry ClassLoader memory leak
Date Wed, 18 Jul 2007 23:11:01 GMT

On Jul 18, 2007, at 3:51 PM, Marc Prud'hommeaux wrote:

>
> On Jul 18, 2007, at 2:52 PM, Kevan Miller wrote:
>
>>
>> On Jul 17, 2007, at 5:32 PM, Marc Prud'hommeaux wrote:
>>
>>>
>>> Correct me if I am wrong, but I think the problem is essentially  
>>> that we have a Map of SomeClass:Object->Object->...->SomeClass,  
>>> and therefore SomeClass can never be garbage collected,  
>>> regardless of whether the keys in the Map are soft or weak, and  
>>> regardless of whether the class' ClassLoader is referenced  
>>> elsewhere or not.
>>>
>>> Another possible solution might be to find every place where we  
>>> have a Class field that could possibly be set to a  
>>> PersistenceCapable, and turn them all into a  
>>> WeakReference<Class>. I haven't checked to see how big a change  
>>> that would be, but it might be one of the simpler solutions (and  
>>> it might be faster than storing the class name as a String and  
>>> doing a Class.forName() every time we need access to the Class  
>>> object).
>>
>> There are two categories of references that are keeping  
>> ClassLoaders alive.
>>
>> (1) References to classes (i.e. Meta fields pcClass, fieldTypes[],  
>> and pcSuper). These can all be wrapped fairly easily using  
>> WeakReferences. I've fixed these, locally.
>
> Sounds good.
>
>> (2) References to objects (i.e Meta.pc). This is a problem.  
>> Meta.pc is the only reference to the PersistenceCapable object.  
>> Since Meta.pc is the only reference to the PersistenceCapable  
>> object, it can't be a WeakReference.

If the only time the WeakReference will be nullified is when the  
class is no longer referenced, isn't it ok to use WeakReference? What  
are the circumstances under which the class is not referenced? If its  
class loader is garbage collected, or no class references the pc class?

The Registry need only keep track of currently used classes. If the  
class loader decides to unload the class, it's ok to remove it from  
the registry, as long as when the class loader needs it again it  
initializes it.

It's been a long time since this architecture was put into place but  
I don't recall any real issues like these...

Craig
>>
>> Any thoughts on fixing the problem with Meta.pc? Does it need to  
>> be an object? Can we generate an instance of PersistenceCapable as  
>> needed?
>
> That is unfortunate. To my knowledge, the only reason we need to  
> keep around an instance of the object is that it is easy to  
> construct new instances using PersistenceCapable.pcNewInstance()  
> without having to use reflection each time or worry about security.  
> I'm not sure of any way around this.
>
>> Or, on a different tack, can PCRegistry be made non-static?  
>> Associated with the PU, for instance?
>
> I personally think it would be nice if we could get rid of  
> PCRegistry being static. However, this part of the problem with the  
> Meta.pc issue in the previous paragraph: the static initializer of  
> any enhanced persistent class will do:
>
>    PCRegistry.register(MyClass.class, pcFieldNames, pcFieldTypes,  
> pcFieldFlags, pcPCSuperclass, "MyClass", new MyClass());
>
> If PCRegistry weren't static, then we wouldn't be able to do this.
>
> Also, I think there are a number of places where we rely on  
> PCRegistry being static for other reasons. I think it would be nice  
> to get rid of these, but that project might be too large a scope  
> just to fix this bug.
>
>
>> As Patrick mentions, another approach is to load OpenJPA in every  
>> application classloader (or a unique OpenJPA ClassLoader per  
>> application ClassLoader). I really don't want to do this... I'd  
>> prefer a private interface to notify you of when ClassLoaders are  
>> no longer valid...
>>
>> --kevan
>>
>>
>>>
>>>
>>> On Jul 17, 2007, at 10:14 AM, Patrick Linskey wrote:
>>>
>>>> Of course, another approach is for Geronimo to trash the  
>>>> classloader
>>>> that OpenJPA was loaded in when the app is undeployed.
>>>>
>>>> -Patrick
>>>>
>>>> On 7/17/07, robert burrell donkin  
>>>> <robertburrelldonkin@gmail.com> wrote:
>>>>> On 7/17/07, Kevan Miller <kevan.miller@gmail.com> wrote:
>>>>> >
>>>>> > On Jul 17, 2007, at 12:15 AM, Craig L Russell wrote:
>>>>> >
>>>>> > >
>>>>> > > On Jul 16, 2007, at 7:48 PM, Kevan Miller wrote:
>>>>> > >
>>>>> > >>
>>>>> > >> On Jul 16, 2007, at 10:26 PM, Pinaki Poddar wrote:
>>>>> > >>
>>>>> > >>> Just to clarify:
>>>>> > >>> I meant it should be the loader that loaded Person.class
 
>>>>> where
>>>>> > >>> Person is
>>>>> > >>> the persistence capable class, *not*
>>>>> > >>>  
>>>>> org.apache.openjpa.enhance.PersistenceCapable.class.getClassLoader
>>>>> > >>> ().
>>>>> > >>>
>>>>> > >>> But testing in multi-classloader environment is required
to
>>>>> > >>> validate any
>>>>> > >>> changes of this nature.
>>>>> > >>
>>>>> > >> Yes, that's how I interpreted your statement. My point
is  
>>>>> that if
>>>>> > >> SSN is a field of Person, the ClassLoader of SSN is not
>>>>> > >> necessarily the ClassLoader for Person. I'd be concerned
 
>>>>> that the
>>>>> > >> Person ClassLoader can't load SSN. In that case, your 

>>>>> technique
>>>>> > >> wouldn't work...
>>>>> > >
>>>>> > > Just to clarify, if SSN is the type of a field of Person, the
>>>>> > > ClassLoader of Person.class must be able to load SSN  
>>>>> (either itself
>>>>> > > or via a parent ClassLoader) or you will have a linkage  
>>>>> error while
>>>>> > > loading Person.
>>>>> >
>>>>> > Craig,
>>>>> > You are correct. The declared type must be loadable. I was  
>>>>> thinking
>>>>> > of an SSNImpl type which need not be. However, that's not really
>>>>> > relevant to the problem at hand...
>>>>> >
>>>>> > Pinaki,
>>>>> > I'm afraid that I may have improperly narrowed your  
>>>>> objectives by
>>>>> > singling out fieldTypes. PCRegistry$Meta.pc and pcSuper could  
>>>>> also
>>>>> > keep Classes/ClassLoaders alive...
>>>>>
>>>>> if there are several places where this may happen then perhaps   
>>>>> try a
>>>>> variant on http://svn.apache.org/repos/asf/jakarta/commons/ 
>>>>> proper/logging/trunk/src/java/org/apache/commons/logging/impl/ 
>>>>> WeakHashtable.java
>>>>>
>>>>> - robert
>>>>>
>>>>
>>>>
>>>> -- 
>>>> Patrick Linskey
>>>> 202 669 5907
>>>
>>
>

Craig Russell
Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
408 276-5638 mailto:Craig.Russell@sun.com
P.S. A good JDO? O, Gasp!


Mime
View raw message