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:

```diff 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 = new Property( 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 { - 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 https://github.com/phetsims/scenery/issues/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 https://github.com/phetsims/scenery/issues/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 https://github.com/phetsims/density-buoyancy-common/issues/168#issuecomment-2158981435, because when working with you and JG sync on https://github.com/phetsims/scenery/issues/1637, we didn't see any other leaks in the area. Then I added the enabledProperty for phet-io support in https://github.com/phetsims/density-buoyancy-common/commit/ffbcd5b16cd9e8442b83620be693bcbbf83162d4 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 https://github.com/phetsims/sun/issues/858