Memory leaks in bamboo components. #66

pixelzoom · 2024-12-09T19:20:00Z

After my experience with #64, I decided to review bamboo components for other memory leaks. Here's what I found:

~~BarPlot: creates a set of Rectangle instances that make be linked to global ProfileColorProperty.~~
SpanNode: creates an ArrowNode instance that may be linked to a global ProfileColorProperty.
UpDownArrowPlot: creates a set of ArrowNode instances that may be linked to global ProfileColorProperty.

These leaks need to be addressed in the update and dispose methods of each component.

These components are not an issue for phetsims/fourier-making-waves#123. FMW does not use SpanNode and UpDownArrowPlot. And while it uses BarPlot, a single instance is reused, and the number of bars is constant.

@samreid FYI.

The text was updated successfully, but these errors were encountered:

samreid · 2024-12-16T20:49:41Z

I set up this test harness to investigate a leak in BarPlot.pointToPaintableFields, but it does not seem to leak at all.

Subject: [PATCH] Use PhetioIDUtilsModule, see https://github.com/phetsims/tandem/issues/316
---
Index: js/demo/DemoBarPlot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/DemoBarPlot.ts b/js/demo/DemoBarPlot.ts
--- a/js/demo/DemoBarPlot.ts	(revision c5be5907ba3f3898d478bcaa15881884b9af346a)
+++ b/js/demo/DemoBarPlot.ts	(date 1734382025876)
@@ -12,7 +12,7 @@
 import Vector2 from '../../../dot/js/Vector2.js';
 import Orientation from '../../../phet-core/js/Orientation.js';
 import MathSymbols from '../../../scenery-phet/js/MathSymbols.js';
-import { Color, Node, NodeOptions, Text, VBox } from '../../../scenery/js/imports.js';
+import { Color, ColorProperty, Node, NodeOptions, Text, VBox } from '../../../scenery/js/imports.js';
 import TextPushButton from '../../../sun/js/buttons/TextPushButton.js';
 import bamboo from '../bamboo.js';
 import BarPlot from '../BarPlot.js';
@@ -62,6 +62,17 @@
       }
     } );
 
+    const myColorProperty = new ColorProperty( Color.blue );
+    setInterval( () => {
+      const m = new BarPlot( chartTransform, createDataSet( 2, 0.1 ), {
+        pointToPaintableFields: ( point: Vector2 ) => {
+          return { fill: myColorProperty };
+        }
+      } );
+      console.log( 'hello' );
+      m.dispose();
+    }, 100 );
+
     const chartClip = new Node( {
       clipArea: chartRectangle.getShape(),
       children: [
Index: js/demo/BambooDemoScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/BambooDemoScreenView.ts b/js/demo/BambooDemoScreenView.ts
--- a/js/demo/BambooDemoScreenView.ts	(revision c5be5907ba3f3898d478bcaa15881884b9af346a)
+++ b/js/demo/BambooDemoScreenView.ts	(date 1734381332066)
@@ -34,37 +34,37 @@
      * {(layoutBounds: Bounds2) => Node} createNode - creates the scene graph for the demo
      */
     const demos: DemoItemData[] = [
-      {
-        label: 'AreaPlot',
-        createNode: layoutBounds => new DemoAreaPlot( { center: layoutBounds.center } )
-      },
+      // {
+      //   label: 'AreaPlot',
+      //   createNode: layoutBounds => new DemoAreaPlot( { center: layoutBounds.center } )
+      // },
       {
         label: 'BarPlot',
         createNode: layoutBounds => new DemoBarPlot( { center: layoutBounds.center } )
       },
-      {
-        label: 'ChartCanvasNode',
-        createNode: layoutBounds => new DemoChartCanvasNode( emitter, { center: layoutBounds.center } )
-      },
-      {
-        label: 'LinearEquationPlot',
-        createNode: layoutBounds => new DemoLinearEquationPlot( { center: layoutBounds.center } )
-      },
-      {
-        label: 'LinePlot',
-        createNode: layoutBounds => new DemoLinePlot( { center: layoutBounds.center } )
-      },
-      {
-        label: 'MultiplePlots', createNode: layoutBounds => new DemoMultiplePlots( { center: layoutBounds.center } )
-      },
-      {
-        label: 'ScatterPlot',
-        createNode: layoutBounds => new DemoScatterPlot( { center: layoutBounds.center } )
-      },
-      {
-        label: 'UpDownArrowPlot',
-        createNode: layoutBounds => new DemoUpDownArrowPlot( { center: layoutBounds.center } )
-      }
+      // {
+      //   label: 'ChartCanvasNode',
+      //   createNode: layoutBounds => new DemoChartCanvasNode( emitter, { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'LinearEquationPlot',
+      //   createNode: layoutBounds => new DemoLinearEquationPlot( { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'LinePlot',
+      //   createNode: layoutBounds => new DemoLinePlot( { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'MultiplePlots', createNode: layoutBounds => new DemoMultiplePlots( { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'ScatterPlot',
+      //   createNode: layoutBounds => new DemoScatterPlot( { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'UpDownArrowPlot',
+      //   createNode: layoutBounds => new DemoUpDownArrowPlot( { center: layoutBounds.center } )
+      // }
     ];
 
     super( demos, {

Perhaps a Rectangle that is cut off from the GC roots also does not retain a reference for fill or stroke Properties? @pixelzoom what do you think?

samreid · 2024-12-16T20:50:08Z

pixelzoom · 2024-12-17T16:57:17Z

Perhaps a Rectangle that is cut off from the GC roots also does not retain a reference for fill or stroke Properties?

I'd be very surprised if that were the case. TickLabelSet was leaking due to Line, and I doubt that scenery would be handling disposable of Line and Rectangle differently. That said...

My bad, BarPlot is not leaking, because it does not pass anything observable to Rectangle:

      const rectangle = new Rectangle( 0, 0, 0, 0 );

SpanNode can leak via arrowNodeOptions passed to ArrowNode:

      const arrowNode = new ArrowNode(
        leftBar.right,
        leftBar.centerY,
        rightBar.left,
        rightBar.centerY,
        this.arrowNodeOptions
      );

UpDownArrowPlot can also leak via arrowNodeOptions passed to ArrowNode:

      const arrowNode = new ArrowNode( 0, 0, 0, 0, this.arrowNodeOptions );

pixelzoom · 2024-12-17T17:02:28Z

Here's a propose (untested) patch for SpanNode and UpDownArrowNode. Please review, feel free to apply and commit if it looks correct.

patch

Subject: [PATCH] visibleProperty phetioReadOnly: true, https://github.com/phetsims/natural-selection/issues/366
---
Index: js/UpDownArrowPlot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/UpDownArrowPlot.ts b/js/UpDownArrowPlot.ts
--- a/js/UpDownArrowPlot.ts	(revision c5be5907ba3f3898d478bcaa15881884b9af346a)
+++ b/js/UpDownArrowPlot.ts	(date 1734454885841)
@@ -67,7 +67,10 @@
     const changedListener = () => this.update();
     chartTransform.changedEmitter.addListener( changedListener );
 
-    this.disposeUpDownArrowPLot = () => chartTransform.changedEmitter.removeListener( changedListener );
+    this.disposeUpDownArrowPLot = () => {
+      chartTransform.changedEmitter.removeListener( changedListener );
+      this.arrowNodes.forEach( node => node.dispose );
+    };
   }
 
   // Sets the dataSet and redraws the plot.
@@ -92,6 +95,7 @@
     // if any data points were removed, remove any extra ArrowNodes
     while ( this.arrowNodes.length > this.dataSet.length ) {
       const arrowNode = this.arrowNodes.pop()!;
+      arrowNode.dispose();
       this.removeChild( arrowNode );
     }
 
Index: js/SpanNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/SpanNode.ts b/js/SpanNode.ts
--- a/js/SpanNode.ts	(revision c5be5907ba3f3898d478bcaa15881884b9af346a)
+++ b/js/SpanNode.ts	(date 1734454885835)
@@ -43,6 +43,7 @@
   private readonly color: string | Color;
   private readonly outerLineLength: number;
   private viewWidth: number;
+  private arrowNode?: ArrowNode;
   private readonly arrowNodeOptions: ArrowNodeOptions;
   private readonly disposeSpanNode: () => void;
 
@@ -92,7 +93,10 @@
     const changedListener = () => this.update();
     chartTransform.changedEmitter.addListener( changedListener );
 
-    this.disposeSpanNode = () => chartTransform.changedEmitter.removeListener( changedListener );
+    this.disposeSpanNode = () => {
+      chartTransform.changedEmitter.removeListener( changedListener );
+      this.arrowNode && this.arrowNode.dispose();
+    };
   }
 
   /**
@@ -116,11 +120,13 @@
 
       //TODO https://github.com/phetsims/bamboo/issues/21 support Orientation.VERTICAL
 
+      this.arrowNode && this.arrowNode.dispose();
+
       // Create double-headed arrow with perpendicular lines at ends to show modelDelta
       const createBar = ( centerX: number ) => new Line( 0, 0, 0, this.outerLineLength, { stroke: this.color, centerX: centerX } );
       const leftBar = createBar( 0 );
       const rightBar = createBar( viewWidth );
-      const arrowNode = new ArrowNode(
+      this.arrowNode = new ArrowNode(
         leftBar.right,
         leftBar.centerY,
         rightBar.left,
@@ -128,7 +134,7 @@
         this.arrowNodeOptions
       );
       const arrowWithBars = new Node( {
-        children: [ leftBar, rightBar, arrowNode ]
+        children: [ leftBar, rightBar, this.arrowNode ]
       } );
 
       //TODO https://github.com/phetsims/bamboo/issues/21 support Orientation.VERTICAL

samreid · 2024-12-18T15:36:24Z

This line does not look correct:

 this.arrowNodes.forEach( node => node.dispose );

Furthermore, creating a harness to try to intentionally trigger a leak SpanNode does not cause a leak in main (without the patch above):

Subject: [PATCH] Use PhetioIDUtilsModule, see https://github.com/phetsims/tandem/issues/316
---
Index: js/demo/DemoBarPlot.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/DemoBarPlot.ts b/js/demo/DemoBarPlot.ts
--- a/js/demo/DemoBarPlot.ts	(revision c5be5907ba3f3898d478bcaa15881884b9af346a)
+++ b/js/demo/DemoBarPlot.ts	(date 1734535988283)
@@ -12,13 +12,14 @@
 import Vector2 from '../../../dot/js/Vector2.js';
 import Orientation from '../../../phet-core/js/Orientation.js';
 import MathSymbols from '../../../scenery-phet/js/MathSymbols.js';
-import { Color, Node, NodeOptions, Text, VBox } from '../../../scenery/js/imports.js';
+import { Color, ColorProperty, Node, NodeOptions, Text, VBox } from '../../../scenery/js/imports.js';
 import TextPushButton from '../../../sun/js/buttons/TextPushButton.js';
 import bamboo from '../bamboo.js';
 import BarPlot from '../BarPlot.js';
 import ChartRectangle from '../ChartRectangle.js';
 import ChartTransform from '../ChartTransform.js';
 import GridLineSet from '../GridLineSet.js';
+import SpanNode from '../SpanNode.js';
 import TickLabelSet from '../TickLabelSet.js';
 import TickMarkSet from '../TickMarkSet.js';
 
@@ -62,6 +63,17 @@
       }
     } );
 
+    const myColorProperty = new ColorProperty( Color.blue );
+    setInterval( () => {
+      const s = new SpanNode( chartTransform, Orientation.HORIZONTAL, Math.PI * 12, new Text( 'hello' ), {
+        arrowNodeOptions: {
+          fill: myColorProperty // Intentional attempt to leak by attaching to the color Property above
+        }
+      } );
+      console.log( 'hello' );
+      s.dispose();
+    }, 100 );
+
     const chartClip = new Node( {
       clipArea: chartRectangle.getShape(),
       children: [
Index: js/demo/BambooDemoScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/demo/BambooDemoScreenView.ts b/js/demo/BambooDemoScreenView.ts
--- a/js/demo/BambooDemoScreenView.ts	(revision c5be5907ba3f3898d478bcaa15881884b9af346a)
+++ b/js/demo/BambooDemoScreenView.ts	(date 1734535667568)
@@ -34,37 +34,37 @@
      * {(layoutBounds: Bounds2) => Node} createNode - creates the scene graph for the demo
      */
     const demos: DemoItemData[] = [
-      {
-        label: 'AreaPlot',
-        createNode: layoutBounds => new DemoAreaPlot( { center: layoutBounds.center } )
-      },
+      // {
+      //   label: 'AreaPlot',
+      //   createNode: layoutBounds => new DemoAreaPlot( { center: layoutBounds.center } )
+      // },
       {
         label: 'BarPlot',
         createNode: layoutBounds => new DemoBarPlot( { center: layoutBounds.center } )
       },
-      {
-        label: 'ChartCanvasNode',
-        createNode: layoutBounds => new DemoChartCanvasNode( emitter, { center: layoutBounds.center } )
-      },
-      {
-        label: 'LinearEquationPlot',
-        createNode: layoutBounds => new DemoLinearEquationPlot( { center: layoutBounds.center } )
-      },
-      {
-        label: 'LinePlot',
-        createNode: layoutBounds => new DemoLinePlot( { center: layoutBounds.center } )
-      },
-      {
-        label: 'MultiplePlots', createNode: layoutBounds => new DemoMultiplePlots( { center: layoutBounds.center } )
-      },
-      {
-        label: 'ScatterPlot',
-        createNode: layoutBounds => new DemoScatterPlot( { center: layoutBounds.center } )
-      },
-      {
-        label: 'UpDownArrowPlot',
-        createNode: layoutBounds => new DemoUpDownArrowPlot( { center: layoutBounds.center } )
-      }
+      // {
+      //   label: 'ChartCanvasNode',
+      //   createNode: layoutBounds => new DemoChartCanvasNode( emitter, { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'LinearEquationPlot',
+      //   createNode: layoutBounds => new DemoLinearEquationPlot( { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'LinePlot',
+      //   createNode: layoutBounds => new DemoLinePlot( { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'MultiplePlots', createNode: layoutBounds => new DemoMultiplePlots( { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'ScatterPlot',
+      //   createNode: layoutBounds => new DemoScatterPlot( { center: layoutBounds.center } )
+      // },
+      // {
+      //   label: 'UpDownArrowPlot',
+      //   createNode: layoutBounds => new DemoUpDownArrowPlot( { center: layoutBounds.center } )
+      // }
     ];
 
     super( demos, {

Please advise.

pixelzoom · 2024-12-18T15:43:42Z

Why does this.arrowNodes.forEach( node => node.dispose ); not look correct?

Why do you think that ArrowNode will not leak if it's linked to a global Property and not disposed?

Let's chat on Zoom instead of assigning this back and forth.

samreid · 2024-12-18T15:47:17Z

Why does this.arrowNodes.forEach( node => node.dispose ); not look correct?

It is not calling the dispose() function (missing parens).

Why do you think that ArrowNode will not leak if it's linked to a global Property and not disposed?

I demonstrated it empirically by running the memory profiler with my patch and observing that there was no memory leak.

Let's chat on Zoom instead of assigning this back and forth.

I'm on zoom at the moment, or please reach out to me on slack to schedule a time.

samreid · 2024-12-18T16:01:48Z

@pixelzoom and I met and reproduced the example above. We were both surprised that there is no leak in the patch. We would like to ask @jonathanolson how this is happening, and whether the same can be done for StringProperty instances.

pixelzoom · 2024-12-18T16:06:51Z

@samreid and I discussed. We suspect that scenery is automatically cleaning up links to color Properties (for fill, stroke,...) but is not cleaning up links to string Properties (Text, RichText). @samreid will confirm with @jonathanolson. If that's the case, then this issue can be closed. And it would be nice to discuss making scenery automatically cleanup string Property links (Text, RichText) since they have been a continual cost and problem while converting sims to dynamic locale.

jonathanolson · 2024-12-18T16:07:07Z

How can I reproduce the leak? I'd be interested to try it out and debug.

pixelzoom · 2024-12-18T16:31:49Z

@jonathanolson @samreid and I discussed. Scenery does not create links to color Properties (fill, stroke, ...) so the "leaks" that I identified herein are a non-issue, and this can be closed without further work. @samreid thanks for demonstrating the lack of leakage and getting us to the bottom of this.

Automatically cleanup string Property links (Text, RichText) is not possible. We can't simply dispose a Text/RichText when it's removed from the scene graph because we may intended to add it again later. So Text/RichText instances (like those in #64 TickLabelSet) do need to be explicitly disposed.

pixelzoom added type:bug Something isn't working type:performance labels Dec 9, 2024

samreid self-assigned this Dec 16, 2024

samreid assigned pixelzoom and unassigned samreid Dec 16, 2024

samreid added the status:ready-for-review label Dec 16, 2024

pixelzoom assigned samreid and unassigned pixelzoom Dec 17, 2024

pixelzoom removed the status:ready-for-review label Dec 17, 2024

samreid assigned pixelzoom and unassigned samreid Dec 18, 2024

pixelzoom assigned samreid Dec 18, 2024

samreid unassigned pixelzoom Dec 18, 2024

pixelzoom closed this as completed Dec 18, 2024

pixelzoom unassigned samreid Dec 18, 2024

pixelzoom added the type:wontfix This will not be worked on label Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memory leaks in bamboo components. #66

Memory leaks in bamboo components. #66

pixelzoom commented Dec 9, 2024 •

edited

Loading

samreid commented Dec 16, 2024

samreid commented Dec 16, 2024

pixelzoom commented Dec 17, 2024

pixelzoom commented Dec 17, 2024

samreid commented Dec 18, 2024

pixelzoom commented Dec 18, 2024 •

edited

Loading

samreid commented Dec 18, 2024

samreid commented Dec 18, 2024

pixelzoom commented Dec 18, 2024

jonathanolson commented Dec 18, 2024

pixelzoom commented Dec 18, 2024 •

edited

Loading

Memory leaks in bamboo components. #66

Memory leaks in bamboo components. #66

Comments

pixelzoom commented Dec 9, 2024 • edited Loading

samreid commented Dec 16, 2024

samreid commented Dec 16, 2024

pixelzoom commented Dec 17, 2024

pixelzoom commented Dec 17, 2024

samreid commented Dec 18, 2024

pixelzoom commented Dec 18, 2024 • edited Loading

samreid commented Dec 18, 2024

samreid commented Dec 18, 2024

pixelzoom commented Dec 18, 2024

jonathanolson commented Dec 18, 2024

pixelzoom commented Dec 18, 2024 • edited Loading

pixelzoom commented Dec 9, 2024 •

edited

Loading

pixelzoom commented Dec 18, 2024 •

edited

Loading

pixelzoom commented Dec 18, 2024 •

edited

Loading