Memory testing issue

[samreid]

It looks like density has a memory leak:

###density fuzzing unbuilt with assertions
6:26am 115MB
6:28am 129MB
6:30am 134MB
6:32am 140MB

[samreid]

Switching back and forth between single block and 2x blocks in density intro screen looks like a leak. I switched back and forth 7x times, and check out all the multiples of 7 in the delta column:

[samreid]

Experimental patch. Corrects one leak, but that one was masking another leak:

Subject: [PATCH] Add REVIEW comment, see https://github.com/phetsims/density-buoyancy-common/issues/123
---
Index: density-buoyancy-common/js/common/view/MassView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/common/view/MassView.ts b/density-buoyancy-common/js/common/view/MassView.ts
--- a/density-buoyancy-common/js/common/view/MassView.ts	(revision e5b7c69b27f7fc8448325b49f34bf5163b39b8bf)
+++ b/density-buoyancy-common/js/common/view/MassView.ts	(date 1717683828811)
@@ -10,7 +10,7 @@
 import Vector3 from '../../../../dot/js/Vector3.js';
 import densityBuoyancyCommon from '../../densityBuoyancyCommon.js';
 import Mass from '../model/Mass.js';
-import { InteractiveHighlighting, KeyboardDragListener, Node, Path } from '../../../../scenery/js/imports.js';
+import { InteractiveHighlighting, KeyboardDragListener, Path } from '../../../../scenery/js/imports.js';
 import { Shape } from '../../../../kite/js/imports.js';
 import MassTagNode from './MassTagNode.js';
 import ConvexHull2 from '../../../../dot/js/ConvexHull2.js';
@@ -35,7 +35,7 @@
 
   private readonly positionListener: () => void;
 
-  private readonly massTagNode: Node | null = null;
+  private readonly massTagNode: MassTagNode | null = null;
   protected readonly tagOffsetProperty: Property<Vector3> = new Property<Vector3>( Vector3.ZERO );
 
   public readonly focusablePath: Path | null;
@@ -119,7 +119,7 @@
         blur: endKeyboardInteraction
       };
 
-      this.focusablePath.addInputListener( {
+      const focusListener = {
         focus: () => {
 
           // We want the newer interaction to take precedent, so tabbing to the item should interrupt the previous mouse drag (if applicable).
@@ -130,7 +130,8 @@
           grabSoundPlayer.play();
           mass.startDrag( mass.matrix.translation );
         }
-      } );
+      };
+      this.focusablePath.addInputListener( focusListener );
 
       const keyboardDragListener = new KeyboardDragListener( {
         // In model units per second
@@ -148,8 +149,11 @@
       this.focusablePath.addInputListener( keyboardDragListener );
 
       this.disposeEmitter.addListener( () => {
+        this.focusablePath?.hasInputListener( blurListener ) && this.focusablePath?.removeInputListener( blurListener );
+        this.focusablePath?.hasInputListener( keyboardDragListener ) && this.focusablePath?.removeInputListener( keyboardDragListener );
+        this.focusablePath?.hasInputListener( focusListener ) && this.focusablePath?.removeInputListener( focusListener );
         keyboardDragListener.dispose();
-        this.focusablePath && this.focusablePath.dispose();
+        this.focusablePath?.dispose();
       } );
     }
 
@@ -181,6 +185,7 @@
     this.massMesh.dispose();
 
     this.massTagNode && this.massTagNode.dispose();
+    console.log( 'mtn disposen' );
 
     super.dispose();
   }
Index: density-buoyancy-common/js/common/view/MassTagNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/common/view/MassTagNode.ts b/density-buoyancy-common/js/common/view/MassTagNode.ts
--- a/density-buoyancy-common/js/common/view/MassTagNode.ts	(revision e5b7c69b27f7fc8448325b49f34bf5163b39b8bf)
+++ b/density-buoyancy-common/js/common/view/MassTagNode.ts	(date 1717682274909)
@@ -67,8 +67,9 @@
       label.dispose();
       visibleProperty.dispose();
       backgroundNode.dispose();
-    } );
 
+      console.log('test')
+    } );
   }
 }
 
Index: scenery/js/listeners/KeyboardDragListener.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/listeners/KeyboardDragListener.ts b/scenery/js/listeners/KeyboardDragListener.ts
--- a/scenery/js/listeners/KeyboardDragListener.ts	(revision 49a4e25f8cc90104745d10ef87b1c1fb7a8e1612)
+++ b/scenery/js/listeners/KeyboardDragListener.ts	(date 1717682925949)
@@ -222,6 +222,7 @@
   private readonly callbackTimer: CallbackTimer;
 
   public constructor( providedOptions?: KeyboardDragListenerOptions ) {
+    console.log('hello')
 
     // Use either dragSpeed or dragDelta, cannot use both at the same time.
     assert && assertMutuallyExclusiveOptions( providedOptions, [ 'dragSpeed', 'shiftDragSpeed' ], [ 'dragDelta', 'shiftDragDelta' ] );
@@ -291,6 +292,8 @@
     // Mutable attributes declared from options, see options for info, as well as getters and setters.
     this._start = options.start;
     this._drag = options.drag;
+
+    console.log(this._drag);
     this._end = options.end;
     this._dragBoundsProperty = ( options.dragBoundsProperty || new Property( null ) );
     this._mapPosition = options.mapPosition;
@@ -733,6 +736,7 @@
     this.interrupt();
     this._disposeKeyboardDragListener();
     super.dispose();
+    delete this._drag;
   }
 }
 
Index: density-buoyancy-common/js/density/view/DensityIntroScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/density/view/DensityIntroScreenView.ts b/density-buoyancy-common/js/density/view/DensityIntroScreenView.ts
--- a/density-buoyancy-common/js/density/view/DensityIntroScreenView.ts	(revision e5b7c69b27f7fc8448325b49f34bf5163b39b8bf)
+++ b/density-buoyancy-common/js/density/view/DensityIntroScreenView.ts	(date 1717682571574)
@@ -32,7 +32,7 @@
 
 export default class DensityIntroScreenView extends DensityBuoyancyScreenView<DensityIntroModel> {
 
-  private rightBox: PrimarySecondaryControlsNode;
+  private readonly rightBox: PrimarySecondaryControlsNode;
 
   public constructor( model: DensityIntroModel, options: DensityBuoyancyScreenViewOptions ) {
 
Index: scenery/js/accessibility/voicing/InteractiveHighlighting.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery/js/accessibility/voicing/InteractiveHighlighting.ts b/scenery/js/accessibility/voicing/InteractiveHighlighting.ts
--- a/scenery/js/accessibility/voicing/InteractiveHighlighting.ts	(revision 49a4e25f8cc90104745d10ef87b1c1fb7a8e1612)
+++ b/scenery/js/accessibility/voicing/InteractiveHighlighting.ts	(date 1717683196649)
@@ -296,6 +296,7 @@
       }
 
       public override dispose(): void {
+        console.log( 'dispose interactive highlight' )
         this.changedInstanceEmitter.removeListener( this._changedInstanceListener );
 
         // remove the activation listener if it is currently attached
Index: density-buoyancy-common/js/common/view/CuboidView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/common/view/CuboidView.ts b/density-buoyancy-common/js/common/view/CuboidView.ts
--- a/density-buoyancy-common/js/common/view/CuboidView.ts	(revision e5b7c69b27f7fc8448325b49f34bf5163b39b8bf)
+++ b/density-buoyancy-common/js/common/view/CuboidView.ts	(date 1717684207746)
@@ -127,13 +127,6 @@
     massDecorationLayer.depthLinesLayer.addChild( this.depthLinesNode );
   }
 
-  /**
-   * Releases references.
-   */
-  public override dispose(): void {
-    super.dispose();
-  }
-
   /**
    * Updates provided geometry arrays given the specific size.
    *

[samreid]

@jessegreenberg and @zepumph identified an issue that was leading to this symptom. @jessegreenberg is taking next steps on that, and I will resume investigation for more sim-specific problems after that is committed

[jessegreenberg]

The fix for this has been committed in phetsims/scenery#1637 and is awaiting review. @samreid would you like to verify?

[samreid]

I can see that elements like HTMLDivElement are no longer being leaked after the fix above. I see plenty of other leaks that may not be related to phetsims/scenery#1637 so I will continue to investigate here.

Here are the "7x" leaks in the latest test:

[samreid]

KeyboardListener has a memory leak when registering to passed-in enabledProperty instances like in

      const keyboardDragListener = new KeyboardDragListener( {
        // In model units per second
        dragSpeed: 3,
        shiftDragSpeed: 0.5,

        // This is needed for keyboard but not for mouse/touch because keyboard input applies deltas, not absolute positions
        transform: INVERT_Y_TRANSFORM,
        drag: ( event, listener ) => {
          mass.updateDrag( mass.matrix.translation.add( listener.modelDelta ) );
        },
        enabledProperty: mass.inputEnabledProperty,
        tandem: Tandem.OPT_OUT
      } );

[samreid]

After the fixes above, density screen 1 memory testing tapers off after about 15 minutes:

[zepumph]

The above fix makes so much sense. I was really confused about your report in #168 (comment), because when working with you and JG sync on phetsims/scenery#1637, we didn't see any other leaks in the area. Then I added the enabledProperty for phet-io support in ffbcd5b and likely caused this all over again. Thanks for investigating. It is a good reminder that despite the work we do here, we will want to do a memory test as part of RC as well (which I think is already done by QA, just noting here for completeness).

[samreid]

Next we have a leak related to touches being saved by buttons. Here is the memory trace for buoyancy all screens fuzzing:

// 3:04pm 189MB
//3:10pm 195MB
//3:13pm 224MB
//3:17pm 256MB

[samreid]

At least one leak seems related to the startEvent which was introduced in phetsims/sun#858

[samreid]

From #123 a reminder to be memory leak testing in phet-io brand.

[samreid]

There is a significant probability that phetsims/buoyancy#67 will affect the memory leaks. Shall we go on old until that is done?

[samreid]

In a few minutes, we leaked 959 GrabReleaseCueNodes:

[samreid]

Each screen is now fuzzing without a memory leak. Let's reschedule for right before dev test.

[samreid]

Fuzzing buoyancy still seems leak-free:

[zepumph]

Let's close this issue!

[samreid]

Reopening based on #316 (comment)

In one run, we seem to have leaked 22,452 PDOMDisplaysInfo instances.

[samreid]

Even the dev test may have a slight memory leak? https://phet-dev.colorado.edu/html/buoyancy/1.2.0-dev.4/phet/buoyancy_all_phet.html?fuzz

[samreid]

I recommend discussing this patch with @zepumph to decide what we want to keep. Note: After this there remain other memory leaks I haven't been able to stop yet.

Subject: [PATCH] Convert to *.ts, see https://github.com/phetsims/density-buoyancy-common/issues/377
---
Index: scenery-phet/js/keyboard/KeyNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/keyboard/KeyNode.ts b/scenery-phet/js/keyboard/KeyNode.ts
--- a/scenery-phet/js/keyboard/KeyNode.ts	(revision 47ff2e1a93171b425349f2f898eb0065a7e5dbb7)
+++ b/scenery-phet/js/keyboard/KeyNode.ts	(date 1724988565126)
@@ -109,7 +109,7 @@
       lineWidth: options.lineWidth
     } );
 
-    keyIcon.boundsProperty.link( () => {
+    const listener = () => {
 
       // scale down the size of the keyIcon passed in if it is taller than the max height of the icon
       let heightScalar = 1;
@@ -144,7 +144,8 @@
       backgroundShadow.setRectBounds( content.bounds.shiftedXY(
         options.xShadowOffset, options.yShadowOffset ) );
       whiteForeground.setRectBounds( content.bounds );
-    } );
+    };
+    keyIcon.boundsProperty.link( listener );
 
     // children of the icon node, including the background shadow, foreground key, and content icon
     options.children = [
@@ -157,6 +158,10 @@
     ];
 
     super( options );
+
+    this.disposeEmitter.addListener( () => {
+      keyIcon.boundsProperty.unlink( listener );
+    } );
   }
 }
 
Index: density-buoyancy-common/js/common/view/MassView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/density-buoyancy-common/js/common/view/MassView.ts b/density-buoyancy-common/js/common/view/MassView.ts
--- a/density-buoyancy-common/js/common/view/MassView.ts	(revision 7946d593dab68270831e960b25e0e372513e90a5)
+++ b/density-buoyancy-common/js/common/view/MassView.ts	(date 1724986101273)
@@ -150,6 +150,7 @@
 
       const dragCueBoundsProperty = new Property( Bounds2.create( mass.getBounds() ) );
 
+      const wasdCueNode = new WASDCueNode( dragCueBoundsProperty );
       this.grabDragInteraction = new GrabDragInteraction( this.focusablePath, keyboardDragListener, {
         onGrab( event ) {
 
@@ -172,7 +173,7 @@
           mass.interruptedEmitter.hasListener( endKeyboardInteraction ) && endKeyboardInteraction();
         },
         showDragCueNode: () => mass.grabDragCueModel.shouldShowDragCue,
-        dragCueNode: new WASDCueNode( dragCueBoundsProperty ),
+        dragCueNode: wasdCueNode,
         grabDragCueModel: mass.grabDragCueModel,
         tandem: Tandem.OPT_OUT
       } );
@@ -229,6 +230,7 @@
         this.focusablePath!.dispose();
 
         mass.transformedEmitter.removeListener( myListener );
+        wasdCueNode.dispose();
       } );
     }
     const resetListener = () => {
Index: scenery-phet/js/accessibility/nodes/WASDCueNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/scenery-phet/js/accessibility/nodes/WASDCueNode.ts b/scenery-phet/js/accessibility/nodes/WASDCueNode.ts
--- a/scenery-phet/js/accessibility/nodes/WASDCueNode.ts	(revision 47ff2e1a93171b425349f2f898eb0065a7e5dbb7)
+++ b/scenery-phet/js/accessibility/nodes/WASDCueNode.ts	(date 1725027602569)
@@ -40,20 +40,24 @@
   right: Math.PI / 2
 };
 
+let count = 0;
+
 export default class WASDCueNode extends Node {
-  protected wNode: Node;
-  protected aNode: Node;
-  protected sNode: Node;
-  protected dNode: Node;
+  protected readonly wNode: Node;
+  protected readonly aNode: Node;
+  protected readonly sNode: Node;
+  protected readonly dNode: Node;
 
   public constructor( boundsProperty: TReadOnlyProperty<Bounds2> ) {
 
+    count++;
+    console.log(count);
     super();
 
-    this.wNode = this.createMovementKeyNode( 'up' );
-    this.aNode = this.createMovementKeyNode( 'left' );
-    this.sNode = this.createMovementKeyNode( 'down' );
-    this.dNode = this.createMovementKeyNode( 'right' );
+    this.wNode = WASDCueNode.createMovementKeyNode( 'up' );
+    this.aNode = WASDCueNode.createMovementKeyNode( 'left' );
+    this.sNode = WASDCueNode.createMovementKeyNode( 'down' );
+    this.dNode = WASDCueNode.createMovementKeyNode( 'right' );
 
     const directionKeysParent = new Node( {
       children: [
@@ -66,22 +70,35 @@
 
     this.addChild( directionKeysParent );
 
-    boundsProperty.link( bounds => {
+    const boundsListener = ( bounds: Bounds2 ) => {
 
       // place the direction cues relative to the object bounds
       this.wNode.centerBottom = bounds.getCenterTop().plusXY( 0, -KEY_SPACING );
       this.aNode.rightCenter = bounds.getLeftCenter().plusXY( -KEY_SPACING, 0 );
       this.sNode.centerTop = bounds.getCenterBottom().plusXY( 0, KEY_SPACING + SHADOW_OFFSET );
       this.dNode.leftCenter = bounds.getRightCenter().plusXY( KEY_SPACING + SHADOW_OFFSET, 0 );
+    };
+    boundsProperty.link( boundsListener );
+
+    this.disposeEmitter.addListener( () => {
+      boundsProperty.unlink( boundsListener );
+
+      // Necessary to detach from the icon bounds
+      this.wNode.dispose();
+      this.aNode.dispose();
+      this.sNode.dispose();
+      this.dNode.dispose();
+
+      count--;
+      console.log(count);
     } );
   }
-
 
   /**
    * Create a node that looks like a keyboard letter key next to an arrow indicating the direction the balloon
    * would move if that key is pressed.
    */
-  private createMovementKeyNode( direction: 'up' | 'down' | 'left' | 'right' ): Node {
+  private static createMovementKeyNode( direction: 'up' | 'down' | 'left' | 'right' ): Node {
 
     // create the arrow icon
     const arrowShape = new Shape();
Index: chipper/js/initialize-globals.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/chipper/js/initialize-globals.js b/chipper/js/initialize-globals.js
--- a/chipper/js/initialize-globals.js	(revision a3f46af15234cd27121cdebefef316470004ff96)
+++ b/chipper/js/initialize-globals.js	(date 1724988779651)
@@ -151,6 +151,10 @@
       public: true
     },
 
+    simCounter: {
+      type: 'flag'
+    },
+
     /**
      * enables debugger commands in certain cases like thrown errors and failed tests.
      */
@@ -165,7 +169,6 @@
      */
     dev: { type: 'flag' },
 
-
     /**
      * sets all modal features of the sim as disabled. This is a development-only parameter that can be useful in
      * combination with fuzz testing. This was created to limit the amount of time fuzz testing spends on unimportant
Index: joist/js/simCounter.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/simCounter.ts b/joist/js/simCounter.ts
new file mode 100644
--- /dev/null	(date 1725025882756)
+++ b/joist/js/simCounter.ts	(date 1725025882756)
@@ -0,0 +1,35 @@
+// Copyright 2024, University of Colorado Boulder
+
+/**
+ * Utility for counting the launches of a simulation, which is helpful when counting crashes during
+ * extended fuzzing. Replaces the sim name with a title that indicates the run number.
+ *
+ * NOTE: There is no easy way to clear the local storage for this value, so it is better to just count the differences.
+ *
+ * @author Sam Reid (PhET Interactive Simulations)
+ */
+
+import TinyProperty from '../../axon/js/TinyProperty.js';
+import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js';
+
+export default function simCounter( simNameProperty: TReadOnlyProperty<string> ): TReadOnlyProperty<string> {
+  if ( phet.chipper.queryParameters.simCounter ) {
+
+    // get the crash count from local storage, or set it to 1.
+    const storedRunCountString = window.localStorage.getItem( 'phet.simCount' );
+
+    let newRunCount: number | null = null;
+    if ( storedRunCountString ) {
+      const storedRunCount = parseInt( storedRunCountString, 10 );
+      newRunCount = storedRunCount + 1;
+    }
+    else {
+      newRunCount = 1;
+    }
+    window.localStorage.setItem( 'phet.simCount', newRunCount.toFixed( 0 ) );
+    return new TinyProperty( simNameProperty.value + ' (Run ' + newRunCount + ')' );
+  }
+  else {
+    return simNameProperty;
+  }
+}
\ No newline at end of file
Index: joist/js/Sim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/Sim.ts b/joist/js/Sim.ts
--- a/joist/js/Sim.ts	(revision 8d73b67cd74cf759647afd6c0a95f8c82dd13955)
+++ b/joist/js/Sim.ts	(date 1725027128202)
@@ -74,6 +74,8 @@
 import isSettingPhetioStateProperty from '../../tandem/js/isSettingPhetioStateProperty.js';
 import StringIO from '../../tandem/js/types/StringIO.js';
 import dotRandom from '../../dot/js/dotRandom.js';
+import simCounter from './simCounter.js';
+import ResetAllButton from '../../scenery-phet/js/buttons/ResetAllButton.js';
 
 // constants
 const PROGRESS_BAR_WIDTH = 273;
@@ -271,6 +273,10 @@
       console.log( 'Debug info:', JSON.stringify( this.getAssertionDebugInfo(), null, 2 ) );
     } );
 
+    if ( phet.chipper.queryParameters.simCounter ) {
+      simNameProperty = simCounter( simNameProperty );
+    }
+
     const options = optionize<SimOptions, SelfOptions, PhetioObjectOptions>()( {
 
       credits: {},
@@ -1132,6 +1138,35 @@
     }
     return info;
   }
+
+  public toggleFuzz(): void {
+    const wasFuzzing = phet.chipper.queryParameters.fuzz;
+
+    phet.chipper.queryParameters.fuzz = !wasFuzzing;
+
+    if ( wasFuzzing ) {
+
+      // Stop fuzzing and reset all screens
+
+      // iterate over ScreenViews and reset them
+      this.screens.forEach( screen => {
+
+        // find all reset all buttons in that screen view
+        const visit = ( node: Node ) => {
+          if ( node instanceof ResetAllButton ) {
+            node[ 'pushButtonModel' ].fire();
+          }
+          else {
+            node.children.forEach( child => visit( child ) );
+          }
+        };
+
+        visit( screen.view );
+      } );
+
+      this.selectedScreenProperty.value = this.simScreens[ 0 ];
+    }
+  }
 }
 
 type LayoutNode = Node & {

After fuzzing with those changes, I observed the letter 'D' had many listeners:

[samreid]

@jonathanolson and I investigated together and concluded that there is no apparent or significant memory leak in main. We see many minor gcs and the major gcs but overall memory seems stable. Closing again.