search

phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
http://scenerystack.org/
MIT License
9 stars 6 forks source link

Memory leaks in VoicingToolbarItem #978

Closed pixelzoom closed 4 months ago

pixelzoom commented 4 months ago

In https://github.com/phetsims/unit-rates/issues/226, heap comparisons show hundreds of links to StringProperties such as simVoicingOnStringProperty, simVoicingOffStringProperty, quickInfoStringProperty -- the strings that appear at the top of VoicingToolbarItem.ts.

There are lots of memory leaks in VoicingToolbarItem. These elements (not a complete list) need to be cleaned up because they are linked to StringProperties.

It's also not clear (to me) whether VoicingToolbarItem is dynamic in practice. If it's not intended to be disposed, you'd be better off deleting disposeVoicingToolbarItem and adding isDisposable: false.

jessegreenberg commented 4 months ago

Thanks, I made things in joist/js/toolbar isDisposable: false and removed dispose functions.

I am not sure how these were leaking before because because the toolbar is not disposed. But unit-rates and other sims are fuzzing without any assertions.

@pixelzoom would you like to review or test anything else?

pixelzoom commented 4 months ago

Changes look good. Closing.