[kepler-dev] change proposal for ptolemy.vergil.tree.EntityTreeModel class.

[Jianwu Wang]

  Hi Christopher,

     Great. Thanks for your work.

     I might still need to bother you for similar things in the 
following leak tracing. Thanks in advance.

Best wishes

Sincerely yours

Jianwu Wang
jianwu at sdsc.edu
http://users.sdsc.edu/~jianwu/

Assistant Project Scientist
Scientific Workflow Automation Technologies (SWAT) Laboratory
San Diego Supercomputer Center
University of California, San Diego
San Diego, CA, U.S.A.


On 9/27/2010 2:58 PM, Christopher Brooks wrote:
> Hi Jianwu,
> I'm not sure if I responded to this, but I folded your change in to the
> trunk and into the 8.0 branch.
>
> No worries about folding speculative changes like this in.  Tracking down
> memory leaks is a real art, and there are many false paths.
>
> Thanks,
>
> _Christopher
>
> On 9/24/10 6:01 PM, Jianwu Wang wrote:
>> Hi Christopher,
>>
>> I did more explicit cleanup in Kepler code and found the current leak 
>> is gone even without the changes for 
>> ptolemy.vergil.tree.EntityTreeModel class. Another issue is that I 
>> found another error in my
>> changes: '_listenerList.remove(new WeakReference(listener));' will 
>> not really work. I attached a new version for this class.
>>
>> So there are two options for it now. We can either remove all my 
>> changes, or use the attached version. I also saw many similar 
>> listeners in Ptolemy, such as _valueListeners in
>> ptolemy.kernel.util.ConfigurableAttribute and 
>> ptolemy.data.expr.Variable, which do not use weak reference. It might 
>> be good to keep them the same.
>>
>> Sorry for the mess up. I should dig more in Kepler before changing 
>> Ptolemy code.
>>
>> On 9/24/10 10:39 AM, Christopher Brooks wrote:
>>> Hi Jianwu,
>>> Thanks, I folded this into the devel trunk and the 8.0 branch.
>>>
>>> _Christopher
>>>
>>> On 9/24/10 9:54 AM, Jianwu Wang wrote:
>>>> Hi Christopher,
>>>>
>>>> Since WeakReference.get() could return null, we'd better check it 
>>>> after using this function. So I changed this class a little big 
>>>> more. Please find new class from the attachment and check it in if you
>>>> think it is proper. Thanks.
>>>>
>>>> On 9/20/10 6:14 PM, Christopher Brooks wrote:
>>>>> Hi Jianwu,
>>>>>
>>>>> Your change looks ok to me. The diff between what you sent and what
>>>>> is in the tree is below:
>>>>>
>>>>> bash-3.2$ diff ~/Downloads/EntityTreeModelNew.java 
>>>>> EntityTreeModel.java
>>>>> 30d29
>>>>> < import java.lang.ref.WeakReference;
>>>>> 64c63
>>>>> < public class EntityTreeModelNew implements TreeModel {
>>>>> ---
>>>>> > public class EntityTreeModel implements TreeModel {
>>>>> 70c69
>>>>> < public EntityTreeModelNew(NamedObj root) {
>>>>> ---
>>>>> > public EntityTreeModel(NamedObj root) {
>>>>> 82c81
>>>>> < _listenerList.add(new WeakReference(listener));
>>>>> ---
>>>>> > _listenerList.add(listener);
>>>>> 176c175
>>>>> < _listenerList.remove(new WeakReference(listener));
>>>>> ---
>>>>> > _listenerList.remove(listener);
>>>>> 188c187
>>>>> < TreeModelListener listener = (TreeModelListener) 
>>>>> ((WeakReference)listeners.next()).get();
>>>>> ---
>>>>> > TreeModelListener listener = (TreeModelListener) listeners.next();
>>>>> 263c262
>>>>> < private List _listenerList = new LinkedList<WeakReference>();
>>>>> ---
>>>>> > private List _listenerList = new LinkedList();
>>>>>
>>>>>
>>>>> I folded in the change and added a comment:
>>>>>
>>>>> /** Add a listener to this model.
>>>>> * @param listener The listener to add.
>>>>> * @see #removeTreeModelListener(TreeModelListener)
>>>>> */
>>>>> public void addTreeModelListener(TreeModelListener listener) {
>>>>> // In http://bugzilla.ecoinformatics.org/show_bug.cgi?id=4801,
>>>>> // Jianwu Wang found that the _listenerList attribute in
>>>>> // ptolemy.vergil.tree.EntityTreeModel was leaking memory.
>>>>> // This attribute keeps holding some references, so that
>>>>> // instances of EntityTreeModel and its sub-classes can not be
>>>>> // GCed when a window is open and closed.
>>>>> //
>>>>> // When it and its sub-classes are used in Kepler,
>>>>> // addTreeModelListener() and removeTreeModelListener()
>>>>> // functions of EntityTreeModel class are called implicitly.
>>>>> // Jianwu did not find a good way to clean up after a window
>>>>> // is closed. Changing the _listenerList attribute using
>>>>> // WeakReference fixe the leak.
>>>>> _listenerList.add(new WeakReference(listener));
>>>>> }
>>>>>
>>>>> I updated the 8.0 release branch, thanks for the patch!
>>>>>
>>>>> Derik, I think you would want this in the 2.1 release. I'm still 
>>>>> working on deterimining
>>>>> if ptolemy-8.0.0 is what should be shipped with Kepler 2.1.
>>>>>
>>>>> _Christopher
>>>>>
>>>>>
>>>>> On 9/20/10 4:27 PM, Jianwu Wang wrote:
>>>>>> As I mentioned at 
>>>>>> http://bugzilla.ecoinformatics.org/show_bug.cgi?id=4801, I found 
>>>>>> one class might need to be changed for memory leak. It's the 
>>>>>> _listenerList attribute in
>>>>>> ptolemy.vergil.tree.EntityTreeModel. One memory leak I'm tracing 
>>>>>> down is that this attribute keeps holding some references, so 
>>>>>> that instances of EntityTreeModel and its sub-classes can not be 
>>>>>> GCed
>>>>>> when a window is open and closed.
>>>>>>
>>>>>> When it and its sub-classes are used in Kepler, 
>>>>>> addTreeModelListener() and removeTreeModelListener() functions of 
>>>>>> EntityTreeModel class are called implicitly. I haven't find good 
>>>>>> ways to clean
>>>>>> up after a window is closed.
>>>>>>
>>>>>> If changing the _listenerList attribute using WeakReference, it 
>>>>>> will be able to fix the leak.
>>>>>
>>>>
>>>>
>>>
>>
>>
>