search

phetsims / graphing-quadratics

"Graphing Quadratics" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
1 stars 4 forks source link

test for memory leaks (brand=phet) #49

Closed pixelzoom closed 6 years ago

pixelzoom commented 6 years ago

Do memory profiling and test for leaks, similar to what was done in https://github.com/phetsims/equality-explorer/issues/64.

Use a built version.

pixelzoom commented 6 years ago

Test results for:

Minutes Heap Size (MB)
0 39.9
5 71.7
10 88.7
15 104
20 113
25 124
30 137
40 151

Memory continues to grow, so looks like there's a memory leak. This is surprising, because this sim has no dynamically-created instances that participate in observer-observable relationships, or that should require dispose calls.

Looking at heap comparisons, I see nothing that I recognize (possibly because names are mangled). I do see quite a few PHET-iO and a11y properties, and phetioMessageStack.

pixelzoom commented 6 years ago

Running in requirejs mode, I see similar continuous growth of the heap. Looking at heap summary, QuadraticNode consistently has '0' in the "# Deleted" column, so I'll start looking there.

pixelzoom commented 6 years ago

QuadraticNode did indeed have a memory leak, plugged in the above commit. When the saved quadratic changes, so does the associated QuadraticNode, and the old QuadraticNode remained linked to equationsVisibleProperty, a view-specific Property that was added late in implementation.

I re-inspected all calls to link, addInputListener, etc., and I don't see anything else involved that is dynamically created. So I'll proceed with another round of heap testing.

pixelzoom commented 6 years ago

Test results for:

Minutes Heap Size (MB)
0 41.4
5 47.5
10 48.6
15 50.0
20 50.4
25 51.5
30 51.3
40 52.2
50 53.8

Then immediately after pressing the "Collect garbage" button, the heap size drops a tiny bit to 53.4MB.

So we've either stabilized ~53MB, or we have a very small memory leak. I don't see anything that is consistently '0' in the "# Deleted" column, so I'm not sure how to proceed.

pixelzoom commented 6 years ago

This suggests that a slow leak is acceptable, or perhaps even "normal".

Slack dev-public:

Sam Reid [9:56 AM] Memory leak testing Graphing Quadratics PhET-iO over the last 5 minutes doesn’t suggest any serious memory leaks (it’s just the normal slow rising pattern). I also don’t see any tandems being added after the sim starts up, so that’s a good indicator that there are no phet-io related memory leaks.

Chris Malley [9:59 AM] Thanks, that matches what I see. I wasn’t aware of this “normal slow rising pattern”. When I tested Equality Explorer (5 screens), it started at 71MB and then oscillated around 82MB.

pixelzoom commented 6 years ago

Results of testing for a longer period, using

Hours Heap Size (MB)
0 42.0
1 53.3
2 57.0
3 61.1
4 64.2

Note that time is in hours. After the first hour, heap size is growing by ~3.5MB per hour.

@ariel-phet Your call on whether this amount of leakage is acceptable. If not, I suspect that it's typical of most (all?) sims, and I'm going to need some assistance tracking it down.

pixelzoom commented 6 years ago

@ariel-phet and I talked about this via Zoom. This leak is slow enough so that it shouldn't be a problem in practice, and does not prevent us from publishing 1.0. It would be nice to understand where the leak is coming from, and whether it's impacting other sims, so I'll ask other developers about known leaks. Then I'll close this issue.

pixelzoom commented 6 years ago

There is apparently a memory leak in scenery that could be responsible for the leak observed here.

Slack conversation with @chrisklus and @jonathanolson Chris Malley [2:25 PM] Graphing Quadratics memory leak testing is revealing a slow memory leak. While chatting with Ariel about this, he said to ping you guys about a potential scenery leak that was recently discovered during work on sun buttons. Does that ring any bells? Chris Klusendorf [2:26 PM] Yeah, if the memory profile shows Colors or Emitters leaking, that’s related to the scenery issue Chris Malley [2:27 PM] I can’t tell exactly what’s leaking. Under what conditions do Colors and Emitters leak? Or is there a GitHub issue that describes this? Chris Klusendorf [2:29 PM] In snapshot comparisons, the deltas should give some information, right? The leak is from Colors uses in gradients, like buttons There should be a github issue but I don’t think there is one yet, we had just finished finding the source of the problem yesterday Chris Malley [4:20 PM] Comparing 2 heap snapshots taken 20 minutes apart, Emitter and Color are both towards the top of of the list based on #Delta and Alloc. Size. Does this look indicative of the Color/Emitter leak? ![screenshot_114](https://user-images.githubusercontent.com/3046552/46820813-50b04680-cd44-11e8-98ec-5f981682d4f0.jpg) Jonathan Olson [4:35 PM] Potentially looks like it yes Chris Malley [4:37 PM] Graphing Quadratics creates all of its buttons at startup, and does not explicitly create any Colors or Emitters. Can you summarize the problem? Jonathan Olson [4:39 PM] Essentially an internal array keeps growing when a specific case of a gradient with two color stops using the same color Property (when the value changes AND it is displayed, and then later not displayed) Chris Malley [4:40 PM] I’m still trying to parse that :) Is that something that can happen with a plain old sun pseudo-3D button that exists for the lifetime of the sim? Jonathan Olson [4:43 PM] It presumably can as long as it goes back and forth between having drawables and not (like getting removed, maybe screen switches) and one of the gradients changes colors dynamically Chris Malley [4:44 PM] No buttons are removed from the scenegraph. No colors are changed explicitly. But I don’t know what sun buttons are doing internally with colors and gradients. Chris Klusendorf [4:53 PM] I’m not sure how the buttons work either but could something like clicking reset all or the visibility button cause it? Those both have gradients and are changing color on press Chris Malley [4:53 PM] that’s what I was wondering.
pixelzoom commented 6 years ago

I don't understand the nature of the scenery leak and whether it is relevant to this sim. Labeling with meeting:developer to see if anyone can clarify for me.

chrisklus commented 6 years ago

@pixelzoom https://github.com/phetsims/scenery/issues/879

pixelzoom commented 6 years ago

phetsims/scenery#879 is "PaintObserver internal failure and memory leak". And I do see PaintObserver with "#Deleted = 0" in Chrome heap comparisons.

jessegreenberg commented 6 years ago

@ariel-phet said that it is good to understand where the leak is coming from but that this doesn't need to block publication.

pixelzoom commented 6 years ago

10/11/18 dev meeting notes:

pixelzoom commented 6 years ago

phetsims/scenery#879 leak is fixed. Reopening to document whether it fixes the small leak that was observed in this sim.

pixelzoom commented 6 years ago

Results of testing after phetsims/scenery#879 leak was fixed.

Minutes Heap Size (MB)
0 43.4
5 49.2
10 49.8
15 51.1
20 51.6
25 52.2
30 52.5

51.4 MB immediately after pressing the "Collect garbage" button.

Not too different from the results in https://github.com/phetsims/graphing-quadratics/issues/49#issuecomment-428441974, looks like there's still a slow continual increase. Nothing obvious in heap comparisons.

pixelzoom commented 6 years ago

See @jonathanolson's testing in https://github.com/phetsims/scenery/issues/879#issuecomment-430099577. I'm convinced that the main leak (PaintObserver) is fixed, and we're only leaking 250kB/hour with fuzz testing. So I'm happy to close this issue again.