[Biojava-dev] memory leak

[mark.schreiber at group.novartis.com]

Looking at this further.

The parent SymbolList adds ChangeListeners to all it's children that are 
created during subList() or subStr() operations. The children of these 
subXXX operations are views on the parent. Any Edit operations performed 
on them are notified to the parent and applied to the parent (who knew? 
how cool!). So that answers the question about what they are for and also 
why they are not cleaned up.

Following your clue about the reapGarbageListeners() method your correct 
that it is never called. The only place it can be called is from 
ChangeSupport.firePreChangeEvent or ChangeSupport.firePostChangeEvent. 
Because nothing we are doing in the loop is actually causing anything to 
change there is no chance for reapListeners() to be called.

To test this out I changed the loop to:

    while(true){
      SymbolList sl2 = sl.subList(1,12);
      Edit e = new Edit(4, DNATools.getDNA(), DNATools.g());
      sl2.edit(e);
    }

This forces a changeEvent and results in garbageCollection of the 
WeakReferences and bingo, no more memory leak. 

So, there needs to be some mechanism by which reapGarbageListeners() can 
be called without the need for a changeEvent. I'm thinking that the 
finalize() method of SimpleSymbolList might call it but this is not 
currently possible as the reapListeners() method is protected and in a 
different package. Maybe the finalize() method could throw a dummy 
changeEvent as a bit of a hack and to signal to the world it is dying.

Matthew or Thomas, Any ideas??

- Mark






Rainer Pudimat <rpudimat at informatik.uni-jena.de>
01/27/2005 11:19 PM

 
        To:     biojava-dev at biojava.org, Mark Schreiber/GP/Novartis at PH
        cc: 
        Subject:        Re: [Biojava-dev] memory leak


Hello Mark,

I have tried your suggested change in ChangeSuppot.removeChangeListener, 
but without positive effect on my heap.

The problem is that the condition
if( (listeners[i].get() == cl) && (types[i] == ct) )
is never fullfilled so your change is never executed.

But anyway, it seems the right place to look for.
Inspired by the method next to removeChangeListener
(reapGarbageListener) I tried to test what Objects
to which References listeners[i] points to, are actually
null:

if( ((listeners[i].get() == cl) && (types[i] == ct)) || 
listeners[i].get() == null )

This is true in many cases.

On the other hand, the method reapGarbageListener was never
executed, independent of the number of
System.gc() calls.

Rainer.

mark.schreiber at group.novartis.com schrieb:
> The problem seems to be in ChangeSupport.removeChangeListener()
> 
>   /**
>    * Remove a listener that was interested in a specific types of 
changes.
>    *
>    * @param cl  a ChangeListener to remove
>    * @param ct  the ChangeType that it was interested in
>    */
>   public void removeChangeListener(ChangeListener cl, ChangeType ct) {
>     synchronized(this) {
>       for(int i = 0; i < listenerCount; i++) {
>         if( (listeners[i].get() == cl) && (types[i] == ct) ) {
>           listenerCount--;
>           System.arraycopy(listeners, i+1, listeners, i, (listenerCount 
- 
> i));
>           System.arraycopy(types, i+1, types, i, (listenerCount - i));
>           return;
>         }
>       }
>     }
> 
> I think that somewhere there needs to be a nullification of the value at 

> listeners[i]. This is a classic memory leak problem outlined in Item 5 
of 
> Joshua Bloch's Effective Java. I think that the loop should be:
> 
> 
>   public void removeChangeListener(ChangeListener cl, ChangeType ct) {
>     synchronized(this) {
>       for(int i = 0; i < listenerCount; i++) {
>         if( (listeners[i].get() == cl) && (types[i] == ct) ) {
> 
>           //prevent memory leak
>           listeners[i] = null;
> 
>           listenerCount--;
>           System.arraycopy(listeners, i+1, listeners, i, (listenerCount 
- 
> i));
>           System.arraycopy(types, i+1, types, i, (listenerCount - i));
>           return;
>         }
>       }
>     }
> 
> Does this look correct to everyone? I'm still a little concerned about 
> that HashMap though as I don't see it here. It also doesn't address the 
> question of what are all these ChangeListeners needed for (but then I'm 
no 
> expert on the Changeable API).
> 
> - Mark
> 
> 
> 
> 
> 
> Mark Schreiber/GP/Novartis at PH
> Sent by: biojava-dev-bounces at portal.open-bio.org
> 01/27/2005 01:25 PM
> 
> 
>         To:     Michael Heuer <heuermh at acm.org>
>         cc:     rpudimat at informatik.uni-jena.de, 
biojava-dev at biojava.org, Michael Heuer 
> <heuermh at shell3.shore.net>, (bcc: Mark Schreiber/GP/Novartis)
>         Subject:        Re: [Biojava-dev] memory leak
> 
> 
> 
>>   synchronized(this) {
>>     growIfNecessary();
>>     types[listenerCount] = ct;
>>     listeners[listenerCount] = new WeakReference(cl);
>>     listenerCount++;
>>   }
> 
> 
> 
> This may be the source of the problem. The thing the WeakReference 
points 
> to is released when memory runs low but a reference to the WeakReference 

> is being maintained (possibly in the listeners[]).
> 
> - Mark
> _______________________________________________
> biojava-dev mailing list
> biojava-dev at biojava.org
> http://biojava.org/mailman/listinfo/biojava-dev
> 
> 
>