phetsims / unit-rates

"Unit Rates" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 2 forks source link

Memory test for 1.1 release #226

Open pixelzoom opened 3 weeks ago

pixelzoom commented 3 weeks ago

For #224

pixelzoom commented 3 weeks ago

Memory test results with https://phet-dev.colorado.edu/html/unit-rates/1.1.0-dev.7/phet/unit-rates_all_phet.html, heap snapshots taken at 1-minute intervals. This sim has a huge memory leak. It looks like most of them are related to #222, and failing to dispose of Nodes that are linked to StringProperty and ColorProperty.

screenshot_3400
pixelzoom commented 3 weeks ago

A rather serious leak in sun ButtonNode was fixed in https://github.com/phetsims/sun/issues/887.

pixelzoom commented 3 weeks ago

After all of the above changes, the sim still has a big memory leak. Looking at heap comparisons, I suspect that it's related to a11y/PDOM, but I don't have time to investigate. This sim dynamically instantiates the majority of the user interface, and (in the future) that will be problematic for PhET-iO instrumentation. So in https://github.com/phetsims/unit-rates/commit/13a30e5549dea59d1f57681eae38383da52af18e, I converted most of the sim to static instantiation, where UI components exist for the lifetime of the sim, and their visibilty changes based on what scene is selected.

The few things that are still dynamically instantiated and disposed can be identified by searching for ": () => void"

model:

view:

pixelzoom commented 3 weeks ago

After making the fixes and improvements linked above, I tested using https://phet-dev.colorado.edu/html/unit-rates/1.1.0-dev.8/phet/unit-rates_all_phet.html. Heap snapshots are shown below. Snapshots 1-9 were taken at 1-minute intervals. Snapshot 10 was taken 10 minutes after Snapshot 9. The sim is still leaking memory, but much more slowly. Heap size never decreases, and continues an upward trajectory.

screenshot_3401
pixelzoom commented 3 weeks ago

I am seeing InterativeHighlighting _onInteractiveHighlightingEnabledChange many times in heap comparisons, so I discussed with @jessegreenberg, in case that seemed significant. Nothing immediately came to mind, but he'll take a look.

I ran a test where I opened and closed KeypadPanel 3x between heap snapshots. There are 13 buttons in KeypadPanel, and heap comparisons reported +39 (3 x 13) RectangularRadioButtons. So it seems like something related to those buttons is leaking. Perhaps the gradients reported in https://github.com/phetsims/sun/issues/887?

To determine if KeypadPanel is the culprit, I ran 1.1.0-dev.8 with ?disableModals&fuzz. Snapshots were similar to https://github.com/phetsims/unit-rates/issues/226#issuecomment-2215273426, so that seems to rule out KeypadPanel.

pixelzoom commented 3 weeks ago

To determine if there's a leak in sun buttons (https://github.com/phetsims/sun/issues/887) related to ProfileColorProperty instances in URColors.ts ... There are 7 instances of ProfileColorProperty with suffix ButtonColorProperty. I converted them to static CSS color strings, like 'white' and 'yellow'. While heap size still did not stabilize or go down, it increased more slowly.

jessegreenberg commented 3 weeks ago

I did a little more testing but I couldn't find leaks related to InteractiveHighlighting.

I ran the sim with ?brand=phet&ea&debugger&screens=1&disableModals&supportsDynamicLocale=false&fuzz

And still saw the leak. I see a lot of Color instances and things related to SVG in the summary. I noticed that there is a recent commit in scenery related to SVG/paint: https://github.com/phetsims/scenery/commit/4e2d3340df12a06b8b2bfeb597bfaf65a33be866

Checking out the SHA before it and running with the same query parameters makes the leak go away. Memory is stable at 88.8 MB.

pixelzoom commented 3 weeks ago

Thanks @jessegreenberg !!

https://github.com/phetsims/scenery/commit/4e2d3340df12a06b8b2bfeb597bfaf65a33be866 was an attempt to address a performance issue with sun buttons for https://github.com/phetsims/faradays-electromagnetic-lab/issues/180. I'll do a PSA and notify @jonathanolson via Slack.

jonathanolson commented 3 weeks ago

Fixed the SVGSelfDrawable leak that my earlier commit exposed. @pixelzoom let me know if this looks like it resolves things? With this patch, I can't reproduce https://github.com/phetsims/sun/issues/887 (and it makes sense that this leak would cause a leak of RadialGradients).

jonathanolson commented 3 weeks ago

@pixelzoom can you verify?

pixelzoom commented 3 weeks ago

Verified that the memory leak is resolved. Tested using (1.1.0-dev.9)[https://phet-dev.colorado.edu/html/unit-rates/1.1.0-dev.9/phet/unit-rates_all_phet.html?fuzz], heap snapshots at 1-minute intervals are shown below. Heap size stabilizes at ~93 MB, and was observed to decrease a couple of times.

To summarize, there were 3 types of leaks that were addressed:

  1. Dispose of Text that is linked to a LocalizedStringProperty.
  2. Dispose of Node that is linked to ProfileColorProperty.
  3. Common-code leaks (ButtonNode and SVGDrawable).

I'll leave this issue open for verification during QA dev testing.

screenshot_3407
pixelzoom commented 2 weeks ago

Please verify in dev test https://github.com/phetsims/qa/issues/1111. The latest memory test is in https://github.com/phetsims/unit-rates/issues/226#issuecomment-2218265330.