[Biojava-dev] memory leak

Sun Jan 30 19:56:09 EST 2005

Irrespective of this there is still a bug. The problem is when the 
SymbolList is disposed of the ChangeSupport keeps a soft reference to a 
null object. The problem seems to be that this line in 
removeChangeListener() is never fulfilled so the clean up code is never 
executed.

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

It looks as though the listeners and types arrays get out of synch 
somewhere and so the condition is never (or maybe rarely) fulfilled.

- Mark

Matthew Pocock <matthew.pocock at ncl.ac.uk>
Sent by: biojava-dev-bounces at portal.open-bio.org
01/28/2005 05:48 PM

        To:     Mark Schreiber/GP/Novartis at PH
        cc:     biojava-dev at biojava.org, td2 at sanger.ac.uk, Rainer Pudimat 
<rpudimat at informatik.uni-jena.de>
        Subject:        Re: [Biojava-dev] memory leak

Ah - do we have a parent-child inner/outer class hard/soft reference 
problem? If you want the sublist to be managed independantly of the 
parent, you should realy copy it. Is there a method in SymbolListTools 
or SequenceTools that does this?

Matthew

mark.schreiber at group.novartis.com wrote:

>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
>>
>>
>>
>> 
>>
>
>
>
>
>
> 
>

_______________________________________________
biojava-dev mailing list
biojava-dev at biojava.org
http://biojava.org/mailman/listinfo/biojava-dev