Project

General

Profile

Actions

Bug #7190

closed

memory leak due to hierarchy listeners when cloning

Added by Daniel Crawl almost 7 years ago. Updated almost 7 years ago.

Status:
Resolved
Priority:
Normal
Category:
-
Target version:
-
Start date:
05/11/2017
Due date:
% Done:

0%

Estimated time:
Spent time:
Bugzilla-Id:

Description

When a model is cloned, some NamedObjs in the new model have hierarchy listeners from the original model, which prevents the new model from being garbage collected.

In the usecase I'm debugging, the toplevel Composites in both the original and new models are added as hierarchy listeners to PortParameters, but only the toplevel in the new model should be added. The original toplevel is added since clone() and setContainer() are called for the PortParameter before its container actor has been moved to the new toplevel.

I don't know the best approach to fix this. Making the listeners weak references would fix the memory leaks, but wrong listeners would still receive hierarchy change events.


Files

leak.java (691 Bytes) leak.java Daniel Crawl, 05/12/2017 05:42 PM
leakConst.java (713 Bytes) leakConst.java Christopher Brooks, 05/17/2017 05:27 PM
Screen Shot 2017-05-17 at 3.03.05 PM.png (268 KB) Screen Shot 2017-05-17 at 3.03.05 PM.png jvisualVm showing leak class with Ramp Christopher Brooks, 05/17/2017 05:36 PM
Actions #1

Updated by Christopher Brooks almost 7 years ago

This is an interesting issue. To pursue it would require a test case.

We've seen plenty of issues with cloning of actors in the past. What happens is that if an actor does not properly clone its fields, then we tend to see non-deterministic behavior around Actor-Oriented classes.

This issue seems to be a different level as it involves ptolemy/kernel/util/HierarchyListener.java and NamedObj.

Actions #2

Updated by Daniel Crawl almost 7 years ago

I'm happy to provide a test case - what's the best format?

Actions #3

Updated by Christopher Brooks almost 7 years ago

A simple model and a description of what to do would be sufficient.

Actions #4

Updated by Daniel Crawl almost 7 years ago

I'm attaching a program that illustrates the leak. It creates a simple model containing Ramp, clones the model 100 times, then sleeps. A reference is kept to the original model, but not the clones, so they should be gc'd.

Once the program sleeps, you should see 101 Ramp instances in the heap: one in the original model and one for each clone. According to jvisualvm, the nearest gc root for all the Ramp instances is _hierarchyListeners in the same CompositeActor instance (the original model).

Actions #5

Updated by Daniel Crawl almost 7 years ago

Actions #6

Updated by Christopher Brooks almost 7 years ago

It seems like when we clone, it is not sufficient to set _hierarchyListeners to null in NamedObj, we also need to look at the container and remove the listeners. setContainer() should do this for us.

Actions #7

Updated by Christopher Brooks almost 7 years ago

Interestingly, if I edit leak.java and replace Ramp with Const, then I don't have the leak. See the attached file, leakConst.java

To replicate,
1) Download leakConst.java to $PTII
2) javac leakConst.java
3) java leakConst &
4) jvisualvm
5) In jvisualvm, select the leakConst process
6) Then select Sampler
7) Then select Memory
8) Then select Perform GC
9) Heap Dump
10) Classes
11) In the entry at the bottom, enter Const
12) Find ptolemy.actor.lib.Const and note that there is only 1 instance.

If you do the above for leak.java, which uses Ramp, you will see 101 instances of Ramp.

One key insight is that with Ramp, jvisualvm is showing that the step PortParameter is causing the leak (See the attached screenshot). Perhaps because Const has no PortParameters, Const does not have this problem?

Actions #8

Updated by Christopher Brooks almost 7 years ago

Ok, now I see Dan's text:

In the usecase I'm debugging, the toplevel Composites in both the original and new models are added as hierarchy listeners to PortParameters, but only the toplevel in the new model should be added. The original toplevel is added since clone() and setContainer() are called for the PortParameter before its container actor has been moved to the new toplevel.
I don't know the best approach to fix this. Making the listeners weak references would fix the memory leaks, but wrong listeners would still receive hierarchy change events.

I agree with the above analysis.

AbstractInitializableParameter.java has a setContainer() method that is invoking addHierarchyListener and removeHierarchyListener. PortParameter extends AbstractInitializabelParameter.

Actions #9

Updated by Christopher Brooks almost 7 years ago

I'm not sure if this is right, but it solves the problem and passes tests.

I modified NamedObj.clone(Workspace) so that it detects the problem and removes listener. See below for the code.

I think we have a larger problem here in part because we don't have setContainer() in NamedObj. In this case, if the container of the container of the parameter is the same as the container of the original master NamedObj, then shouldn't we set the container of the container of the parameter to something else? Maybe this happens later.

Anyway, let's leave this bug open until the nightly build runs and we see if anything breaks.

                while (parameters.hasNext()) {
                    Attribute parameter = (Attribute) parameters.next();
                    Attribute newParameter = (Attribute) parameter
                            .clone(workspace);

                    try {
                        newParameter.setContainer(newObject);
                        // PortParameters are                                                                                                               
                        // AbstractInitializableParameters that end up                                                                                      
                        // adding HierarchyListeners to the                                                                                                 
                        // initializable container of the actor.  This                                                                                      
                        // results in a memory leak, so we remove                                                                                           
                        // parameters that are HierarchyListeners.                                                                                          
                        // See https://projects.ecoinformatics.org/ecoinfo/issues/7190                                                                      
                        if (newParameter instanceof HierarchyListener) {
                            if (newParameter.getContainer() != null) {
                                NamedObj newContainerContainer = newParameter.getContainer().getContainer();
                                NamedObj oldContainer = getContainer();
                                // FIXME: This is probably a serious                                                                                        
                                // problem that the container of the                                                                                        
                                // container of the parameter in the                                                                                        
                                // clone is the same as the container                                                                                       
                                // of original master NamedObj.  Part                                                                                       
                                // of the issue here is that NamedObj                                                                                       
                                // does not have setContainer().  What                                                                                      
                                // we would really like to do is to                                                                                         
                                // set the container to null during                                                                                         
                                // cloning.                                                                                                                 
                                if (newContainerContainer == oldContainer) {
                                    newContainerContainer.removeHierarchyListener((HierarchyListener)newParameter);
                                }
                            }
                        }
                    } catch (KernelException exception) {
                        throw new CloneNotSupportedException(
                                "Failed to clone attribute " 
                                        + parameter.getFullName() + ": " 
                                        + exception);
                    }
                }
            }
Actions #10

Updated by Daniel Crawl almost 7 years ago

Thanks for the fix, Christopher. I verified the memory leak is going, so feel free to close the bug once the nightly tests run.

Actions #11

Updated by Daniel Crawl almost 7 years ago

Oops, that should be: memory leak is gone, not going.

Actions #12

Updated by Christopher Brooks almost 7 years ago

I don't seem to have permission to close bugs, but this bug can be closed now. The nightly build seems to be working just fine.

Actions #13

Updated by Daniel Crawl almost 7 years ago

  • Status changed from New to Resolved
Actions

Also available in: Atom PDF