memory leaks in Dialog

[pixelzoom]

Multiple leaks in joist.Dialog:

97  content.addEventListener( 'bounds', updateTitlePosition );
98  titleNode.addEventListener( 'localBounds', updateTitlePosition );
...
133 options.title.addEventListener( 'bounds', updateClosePosition );

Dialog doesn't own any of these things that it's adding listeners to; content is a constructor argument, and titleNode is an alias for options.title. Dialog also has no dispose function to remove the listeners, so it would technically fail a code review.

Assigning to @ariel-phet to prioritize and assign.

[ariel-phet]

@samreid and @pixelzoom please discuss and collaborate on improving dialog. We are adding dialogs in several sims with increased functionality so now would be a good time to improve the implementation.

How about come up with a plan and lets discuss a developer meeting, and we can prioritize further there.

[samreid]

Also addEventsListener is deprecated.

[pixelzoom]

@samreid and I discussed the general status of Dialog today via Skype. But we have no plan yet for how to move forward. The first order of business would likely be to triage related issues, evaluate the current implementation, then decide whether to revised the currently implementation or start over.

[pixelzoom]

And we should probably create a new issue titled something like "improve Dialog", since this issue is not appropriate for the task that @ariel-phet specified in https://github.com/phetsims/joist/issues/357#issuecomment-248452092.

[pixelzoom]

9/22/16 dev meeting: I'll create an "improve Dialog" issue, link to related issues, look for design meeting plans, etc.

[pixelzoom]

Unassigning, now tracking in the more general issue https://github.com/phetsims/joist/issues/359 ("improve Dialog").

[pixelzoom]

Still an issue in Dialog. Should be addressed when Dialog is revised/rewritten in #359.

[pixelzoom]

Yet another leak, discovered in phetsims/molecule-polarity#33. Dialog doesn't own the {Node} content that is passed to its constructor, and that Node may be reused, as it currently is with OptionsDialog. So content needs to be removed as a child when the Dialog is disposed. Hopefully fixed in phetsims/joist@4c48cf0.

[pixelzoom]

3/29/18 dev meeting: @andrea-phet and I will triage this issue.

[pixelzoom]

Dialog has been moved to sun. So I'm going to move this issue from joist to sun using GitHub's new "Transfer issue" feature.

[jbphet]

Memory leaks in dialogs appears to be the main cause of the issue reported for blackbody-spectrum in phetsims/blackbody-spectrum#72. Here are a couple of screenshot from the memory profile comparison:

I put a debugger statement in Dialog.disposeDialog(), and it didn't seem to get hit during ~20 seconds of fuzz testing nor during directed testing where I opened and closed dialogs manually via the PhET menu. Maybe it's just a matter of calling this function when dialogs go away.

Labeling for dev meeting to see if we can assign someone to follow up on this. It's not a huge memory leak, but we should fix it so that developers aren't spending time tracking this down for every sim that they publish.

[pixelzoom]

@jbphet I don't see any sim-specific Dialogs in blackbody-spectrum, so I'm guessing that the Dialogs that you're referring to in the previous comment are exclusively from the PhET menu. If that's the case, then this is not specific to blackbody-spectrum and is impacting all sims.

[pixelzoom]

@ariel-phet this issue is still assigned to @andrealin. Should it be reassigned?

[jessegreenberg]

In phetsims/joist#424 Dialogs from the PhET menu were made to exist for the life of the sim which is why dispose is not called on them. Callbacks in PhET menu buttons look like

          if ( !updateDialog ) {
            var phetButton = sim.navigationBar.phetButton;
            updateDialog = new UpdateDialog( phetButton );
          }
          updateDialog.show();

[pixelzoom]

Hmmm... If PhET menu dialogs are persistent, then I wonder which dialogs @jbphet is referring to in blackbody-spectrum. #432 (comment) seems to indicate UpdateDialog.

[pixelzoom]

2/21/19 dev meeting:

• @jbphet will look into this.

[jbphet]

I tested this using the "Blast" example sim, since it has very little going on that would cause memory to be allocated. I'll add details below, but my conclusion is that the dialogs are not leaking memory, they just take a while to allocate all that they use. This was done on master as of today.

Below is a screenshot of the rate of leakage on blast. There does seem to be a slow leak during fuzz testing, but when I compared the memory snapshots and spot checked a number of the additional allocations, they all seemed to be associated with Scenery pools. The screenshot shows memory values at 0-5 min of fuzz test sampling at every minute, and then one last snapshot after stopping the test.

I ran a second test where I started the sim, brought up the dialogs that are available from the PhET menu, recorded and snapshot, brought up the dialogs again, and recorded another snapshot. Again, there was some memory allocated, but it all seemed traceable to Scenery pools.

I'm not sure where to go next with this, so I'll mark for dev meeting and we can collectively decide.

[pixelzoom]

Potential memory leak for isShowingProperty #369 fixed in the above commit. Since it's @protected, subclasses might add a listener that creates a leak. isShowingProperty.dispose is now called in dialogDispose.

@jbphet said:

my conclusion is that the dialogs are not leaking memory, they just take a while to allocate all that they use.

I reviewed Dialog and I don't see any of the potential leak sources that existed when this issue created. And since all of our dialogs are currently persistent, I don't know how they could leak. So I think we can close this issue...

There does seem to be a slow leak during fuzz testing, but when I compared the memory snapshots and spot checked a number of the additional allocations, they all seemed to be associated with Scenery pools.

... and create an issue for the potential leak in Scenery pools.

[samreid]

I'm not opposed to further investigation, but the pattern we have seen previously in sims like graphing lines is that it takes 30+ minutes for our memory patterns to stabilize: phetsims/graphing-lines#78 (comment). So I'm not convinced that the data in #432 (comment) indicates a leak. Happy to be proved otherwise, though!

[zepumph]

An idea of another test, manually set the Poolable option maxSize to 0, and see that there would be no slow "tail" increasing the memory as the pools get to their max size.

this will be investigated in phetsims/phet-core#55.

There seems to be no memory leak in Dialog. As of dev meeting today, closing