Memory leak

[jbphet]

While working on the multi-touch issues in #117, I found that there is a significant memory leak in this sim. I don't know whether this was always there, or crept in due to some post-publication changes, but I think it should be addressed. To quantify it, I ran the following test. All of these was done in a Chrome incognito window using an un-built version on master.

• Launch the sim without fuzz enabled and take a snapshot.
• Launch the sim with fuzz enabled, let it run for 30 seconds
• Stop fuzz testing using the console and the command phet.chipper.queryParameters.fuzz=false
• Reset the first screen, then press the "Start Over" button on the 2nd screen and then the reset button, then go to the home screen
• Take a heap snapshot.
• Re-enable fuzzing with the command phet.chipper.queryParameters.fuzz=true
• Allow fuzzing to run for two minutes
• Stop fuzzing, again using phet.chipper.queryParameters.fuzz=false
• Again reset the first screen, then "Start Over" on the 2nd screen, reset it, then go to the home screen
• Take a heap snapshot

Here are the results:

Test	Memory Usage (MB)
Load, no fuzz	23.6
Fuzz for 30 seconds and then reset	90.3
Fuzz for 2 minutes and then reset	251

[jbphet]

I compared the heap snapshots from the 2nd and 3rd tests and sorted by "# Delta" and looked for the first non-common-code class. found that an additional 2,329 instances of MovableShape in the 2nd snapshot. Here's a screenshot of the comparison:

I picked one and examined its "Retainers" (that's what the dev tools calls this, and I think it's basically the reference tree). It looks like these are rooted in AreaBuilderGameChallenge.userShapes:

I'll concentrate the investigation here initially.

[jbphet]

I've spent as much time on this as I can afford (more, actually). It's better, but not perfect. Testing with the commits above yielded:

Test	Memory Usage (MB)
Load, no fuzz	23.5
Fuzz for 30 seconds and then reset	40.1
Fuzz for 2 minutes and then reset	78.5

So, the usage after two minutes is about 1/3 of what it was originally. This is good, but I plan to leave this open so that more investigation can be done if and when this is published from master.

[jbphet]

I decided to revisit this, because I had some concerns that there was an easy way to create a memory leak using the current common code. I was essentially correct in my suspicions. There is code in Path.js that I don't think existed when this sim was originally published and that automatically hooks up a listener to the Shape that is used to create the path. This listener is intended to detect changes to the shape. Because the area-builder sim creates Path instances using instances of Shape that stick around for the life of the sim, if the Path instance isn't disposed from the scene graph, it will leak memory. I tracked down some places where disposal was not occurring and added it, and the memory leak went down to what are probably reasonable levels. Here's a result from one iteration of the test described above:

Test	Memory Usage (MB)
Load, no fuzz	24.0
Fuzz for 30 seconds and then reset	34.0
Fuzz for 2 minutes and then reset	46.7

I'm going to talk this over with @jonathanolson to see if he has any concerns about this. I suspect this is just part of the new reality, but I want to make sure he is aware of the consequences, and that my understanding is correct.

[jonathanolson]

Sounds good to discuss! If it helps, you can call makeImmutable() on a Shape after it's constructed/modified, such that those listeners won't be added by Paths.

[jbphet]

I spoke with @jonathanolson, and he said that the code to hook up shape-change-monitoring listeners has always been there, so it is a bit of a mystery as to why this sim didn't leak memory in the past but does now. By the way, I did test the published version, and it does not appear to leak. We decided that it's not worth the effort to track down a full explanation, and will just fix this and move on.

I added the call to makeImmutable() for the static shapes, and this appears to have improved the situation even more. The test with this in place yields the following results for memory usage:

Test	Memory Usage (MB)
Load, no fuzz	24.0
Fuzz for 30 seconds and then reset	29.9
Fuzz for 2 minutes and then reset	32.2

This seems quite good, so I'm going to go ahead and close.