Memory leak

[pixelzoom]

Reported in phetsims/qa#1066 (comment) for graphing-lines by @KatieWoe:

Two snapshots in and it already looks like there's a pretty big memory leak.

[pixelzoom]

... and in phetsims/qa#1067 (comment) for graphing-slope-intercept by @KatieWoe:

Similar to phetsims/qa#1066, this sim has a large memory leak

[pixelzoom]

I've identified a couple of common-code leaks -- NumberPicker is probably the largest contributor in this sim. But I suspect that most of the leaks are due to disposed elements that are not unlinking from LocalizedStringProperty and ProfileColorProperty.

[pixelzoom]

In the above commits, I fixed a couple of common-code leaks, and many sim-specific leaks related to LocalizedStringProperty and ProfileColorProperty. (These will need to be cherry picked.)

But... the heap size did not change much (see below) so there's more to do here.

Next step will be to add a listener limit to LocalizedStringProperty and ProfileColorProperty, then fuzz test to identify what is linked to them.

[pixelzoom]

I added this to ProfileColorProperty:

  public override link( listener: PropertyLinkListener<Color>, options?: LinkOptions ): void {
    if ( this.tinyProperty.listeners.size > 25 ) {
      debugger;
    }
    super.link( listener, options );
  }

And I added this to LocalizedStringProperty:

  public override link( listener: PropertyLinkListener<string>, options?: LinkOptions ): void {
    if ( this.tinyProperty.listeners.size > 100 ) {
      debugger;
    }
    super.link( listener, options );
  }

Fuzz tested, and discovered a leak in AccessibleNumberSpinner (see phetsims/sun#879) and a few more sim-specific leaks (fixed in e6a8bbf).

I tested with this uncommitted patch to AccessibleNumberSpinner:

Subject: [PATCH] various memory leaks related to LocalizedStringProperty and ProfileColorProperty, https://github.com/phetsims/graphing-lines/issues/153
---
Index: js/accessibility/AccessibleNumberSpinner.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/AccessibleNumberSpinner.ts b/js/accessibility/AccessibleNumberSpinner.ts
--- a/js/accessibility/AccessibleNumberSpinner.ts	(revision 75e22745c05481d1d6b3e80c8d7d2138563df27a)
+++ b/js/accessibility/AccessibleNumberSpinner.ts	(date 1712621584033)
@@ -180,6 +180,7 @@
         this.pdomDecrementDownEmitter.dispose();
 
         this.removeInputListener( accessibleInputListener );
+        this.removePDOMAttribute( 'aria-roledescription' );
       };
     }

Heap size is much better, but it does not yet stabilize or go down:

[pixelzoom]

I identified a few more things that were observing Line.color, which is TColor and may therefore be a Property.

Heap size now stabilizes at ~102 MB for graphing-lines:

It's less clear when graphing-slope-intercept stabilizes, but heap size did decrease twice, so I think this is OK:

So after the leak in AccessNumberSpinner (or ParallelDOM) is fixed in phetsims/sun#879, we should be good to go.

Next is to cherry pick the sim-specific and common-code commits added above.

[pixelzoom]

Here's what needs to be cherry picked so far, from above commits.

scenery-phet

• phetsims/scenery-phet@e3797d4

sun

• phetsims/sun@75e2274

graphing-lines

• 6a59f6f
• e6a8bbf
• 05d7e49
• d4d92dd

[pixelzoom]

In phetsims/sun#879 (comment), @jessegreenberg noted:

For graphing-lines, it does look like RichDragListeners can be created and not disposed for the 'reward' views. So I probably started to see the leak after a while because the fuzzer happened to win a game.

Ugh. I see that RichDragListener needs to be disposed because it owns 2 SoundClips that are added to soundManager. And this sim has 6 subclasses of RichDragListener.

The game reward (GLRewardNode) also looks like its leaking, because some of the rewards are linked to LocalizedStringProperty (eg "x" and "y" in the equations).

I'll tackle those next.

[pixelzoom]

Over in phetsims/sun#879, I confirmed that there's nothing wrong with disposal of AccessibleNumberSpinner, so no changes forthcoming for that.

After the above disposal of RichDragListener and GLRewardNode, there's more improvement. I tested with unbuilt, heap snapshots at 1-minute intervals, 10 minutes between snapshots 10 & 11.

http://localhost:8080/graphing-lines/graphing-lines_en.html?brand=phet&ea&fuzz&debugger

http://localhost:8080/graphing-slope-intercept/graphing-slope-intercept_en.html?brand=phet&ea&fuzz&debugger

[pixelzoom]

Cherry picks for RichDragListener and GLRewardNode leaks were done above.

This is ready for testing in the next RC.

[pixelzoom]

Ready for review in GL 1.4.0-rc2 and GSI 1.2.0-rc2. Please close this issue if it tests OK.

Below are my tests for the RC2 built versions. I took a heap snapshot every 1 minute, until the heap sized stabilized.

https://phet-dev.colorado.edu/html/graphing-lines/1.4.0-rc.2/phet/graphing-lines_en_phet.html?fuzz

https://phet-dev.colorado.edu/html/graphing-slope-intercept/1.2.0-rc.2/phet/graphing-slope-intercept_en_phet.html?fuzz

[Nancy-Salpepi]

Memory tests look good. Snapshots were taken at 1 minute intervals, with 10 minutes between snapshots 10 and 11.

Graphing Lines 1.4.0-rc.2:

Graphing Slope Intercept 1.2.0-rc.2: