Kepler: Activityhttps://projects.ecoinformatics.org/ecoinfo/https://projects.ecoinformatics.org/ecoinfo/ecoinfo/favicon.ico?14691340362017-05-22T23:56:24ZEcoinformatics Redmine
Redmine Bug #7189: memory leak in Managerhttps://projects.ecoinformatics.org/ecoinfo/issues/7189#change-231512017-05-22T23:56:24ZChristopher Brookscxh@eecs.berkeley.edu
<p>I've made adjustments for how we create and delete the shutdown hook. Let's wait a bit to see what happens before closing this one.</p> Bug #7190 (Resolved): memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231502017-05-22T21:51:17ZDaniel Crawldanielcrawl@gmail.comBug #7190: memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231492017-05-22T21:35:45ZChristopher Brookscxh@eecs.berkeley.edu
<p>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.</p> Bug #7191 (Resolved): memory leak in CompositeActor for actor firing listenershttps://projects.ecoinformatics.org/ecoinfo/issues/7191#change-231462017-05-18T21:57:35ZDaniel Crawldanielcrawl@gmail.com
<p>Looks good, thanks.</p> Bug #7191: memory leak in CompositeActor for actor firing listenershttps://projects.ecoinformatics.org/ecoinfo/issues/7191#change-231452017-05-18T21:19:42ZChristopher Brookscxh@eecs.berkeley.edu
<p>Thanks for the patch, I patched CompositeActor.java and updated /ptolemy/module-info/revision.txt.</p>
<p>Feel free to close this after it is validated that the fix is good.</p> Bug #7190: memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231442017-05-18T20:12:49ZDaniel Crawldanielcrawl@gmail.com
<p>Oops, that should be: memory leak is gone, not going.</p> Bug #7191 (Resolved): memory leak in CompositeActor for actor firing listenershttps://projects.ecoinformatics.org/ecoinfo/issues/71912017-05-18T20:10:09ZDaniel Crawldanielcrawl@gmail.com
<p>List of actor firing listeners needs to be null before cloning. I'm attaching a patch.</p> Bug #7190: memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231422017-05-18T20:07:37ZDaniel Crawldanielcrawl@gmail.com
<p>Thanks for the fix, Christopher. I verified the memory leak is going, so feel free to close the bug once the nightly tests run.</p> Bug #7189: memory leak in Managerhttps://projects.ecoinformatics.org/ecoinfo/issues/7189#change-231412017-05-18T17:28:05ZChristopher Brookscxh@eecs.berkeley.edu
<p>I can take a look at this next week, after the dust settles from the other clone() bug.</p> Bug #7190: memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231402017-05-18T17:16:12ZChristopher Brookscxh@eecs.berkeley.edu
<p>I'm not sure if this is right, but it solves the problem and passes tests.</p>
<p>I modified NamedObj.clone(Workspace) so that it detects the problem and removes listener. See below for the code.</p>
<p>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.</p>
<p>Anyway, let's leave this bug open until the nightly build runs and we see if anything breaks.</p>
<pre>
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);
}
}
}
</pre> Bug #7190: memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231392017-05-18T01:02:44ZChristopher Brookscxh@eecs.berkeley.edu
<p>Ok, now I see Dan's text:</p>
<blockquote>
<p>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.<br />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.</p>
</blockquote>
<p>I agree with the above analysis.</p>
<p>AbstractInitializableParameter.java has a setContainer() method that is invoking addHierarchyListener and removeHierarchyListener. PortParameter extends AbstractInitializabelParameter.</p> Bug #7190: memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231382017-05-18T00:39:11ZChristopher Brookscxh@eecs.berkeley.edu
<p>Interestingly, if I edit leak.java and replace Ramp with Const, then I don't have the leak. See the attached file, leakConst.java</p>
<p>To replicate, <br />1) Download leakConst.java to $PTII<br />2) javac leakConst.java<br />3) java leakConst &<br />4) jvisualvm<br />5) In jvisualvm, select the leakConst process<br />6) Then select Sampler<br />7) Then select Memory<br />8) Then select Perform GC<br />9) Heap Dump<br />10) Classes<br />11) In the entry at the bottom, enter Const<br />12) Find ptolemy.actor.lib.Const and note that there is only 1 instance.</p>
<p>If you do the above for leak.java, which uses Ramp, you will see 101 instances of Ramp.</p>
<p>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?</p> Bug #7190: memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231372017-05-17T23:03:47ZChristopher Brookscxh@eecs.berkeley.edu
<p>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.</p> Bug #7190: memory leak due to hierarchy listeners when cloninghttps://projects.ecoinformatics.org/ecoinfo/issues/7190#change-231342017-05-13T00:42:21ZDaniel Crawldanielcrawl@gmail.com
<p>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.</p>
<p>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).</p> Bug #7189: memory leak in Managerhttps://projects.ecoinformatics.org/ecoinfo/issues/7189#change-231332017-05-12T20:26:24ZChristopher Brookscxh@eecs.berkeley.edu
<p>Good point. I'll keep this bug in my queue.</p>