PropertyStateHandler dependency registration leaks memory

[pixelzoom]

While working on memory leaks in phetsims/natural-selection#189, @samreid and I identified a leak in dependencies that are registered with PropertyStateHandler. We identified this by comparing heap snapshots for natural-selection:

In brand=phet, DerivedProperty registers its dependencies like this:

        if ( dependency instanceof Property && this.isPhetioInstrumented() && dependency.isPhetioInstrumented() ) {

          // Dependencies should have taken their correct values before this DerivedProperty undefers, so it will be sure to have the right value.
          // NOTE: Do not mark the beforePhase as NOTIFY, as this will potentially cause interdependence bugs when used
          // with Multlinks. See Projectile Motion's use of MeasuringTapeNode for an example.
          phetioStateHandlerSingleton.registerPhetioOrderDependency( dependency, PropertyStatePhase.UNDEFER, this, PropertyStatePhase.UNDEFER );
        }

But Property (and therefore DerivedProperty) unregisters like this:

    // unregister any order dependencies for this Property for PhET-iO state
    if ( Tandem.PHET_IO_ENABLED  && this.isPhetioInstrumented() ) {
      propertyStateHandlerSingleton.unregisterOrderDependenciesForProperty( this );
    }

Note the difference in the if expressions. Unregistering requires Tandem.PHET_IO_ENABLED, while registering does not. So in brand=phet, dependencies are registered, never unregistered, and memory leaks. (This is also the source of the string arrays of phetioIDs that were identified in phetsims/natural-selection#189 (comment).)

We decided on the following patch, which needs to be reviewed by @zepumph. Like the patch we proposed for Tandem in phetsims/tandem#203, this patch proposes to remove the Tandem.PHET_IO_ENABLED condition.

patch

Index: js/Property.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Property.js	(revision ebad74acf9f07d850708e39ca024f1c4e1bcc40a)
+++ js/Property.js	(date 1598383034096)
@@ -483,7 +483,7 @@
     this.unlinkAll();
 
     // unregister any order dependencies for this Property for PhET-iO state
-    if ( Tandem.PHET_IO_ENABLED && this.isPhetioInstrumented() ) {
+    if ( this.isPhetioInstrumented() ) {
       propertyStateHandlerSingleton.unregisterOrderDependenciesForProperty( this );
     }

Top priority, because this is blocking multiple sims.

[samreid]

The patch looks accurate to me, @zepumph can you please review?

[pixelzoom]

This is blocking ph-scale RC, see phetsims/ph-scale#199. Milestone for 1.4 publication is ??

This is blocking natural-selection prototype RC, see phetsims/natural-selection#205. Milestone for publication is "this week if possible".

[zepumph]

After the below patch, I didn't see anything in my heap comparison relating to the sets in PropertyStateHandler in PhET brand. @samreid do you think it is reasonable to no-op in phet brand for this module? I think I prefer it to the above patch.

Index: js/PropertyStateHandler.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyStateHandler.js	(revision ebad74acf9f07d850708e39ca024f1c4e1bcc40a)
+++ js/PropertyStateHandler.js	(date 1598404471186)
@@ -132,14 +132,16 @@
    * @param {PropertyStatePhase} afterPhase
    */
   registerPhetioOrderDependency( beforeProperty, beforePhase, afterProperty, afterPhase ) {
+    if ( Tandem.PHET_IO_ENABLED ) {
 
-    this.validatePropertyPhasePair( beforeProperty, beforePhase );
-    this.validatePropertyPhasePair( afterProperty, afterPhase );
-    assert && beforeProperty === afterProperty && assert( beforePhase !== afterPhase, 'cannot set same Property to same phase' );
+      this.validatePropertyPhasePair( beforeProperty, beforePhase );
+      this.validatePropertyPhasePair( afterProperty, afterPhase );
+      assert && beforeProperty === afterProperty && assert( beforePhase !== afterPhase, 'cannot set same Property to same phase' );
 
-    const mapPair = this.getMapPairFromPhases( beforePhase, afterPhase );
+      const mapPair = this.getMapPairFromPhases( beforePhase, afterPhase );
 
-    mapPair.addOrderDependency( beforeProperty.tandem.phetioID, afterProperty.tandem.phetioID );
+      mapPair.addOrderDependency( beforeProperty.tandem.phetioID, afterProperty.tandem.phetioID );
+    }
   }
 
   /**
@@ -158,38 +160,40 @@
    * @public
    */
   unregisterOrderDependenciesForProperty( property ) {
-    this.validateInstrumentedProperty( property );
+    if ( Tandem.PHET_IO_ENABLED ) {
+      this.validateInstrumentedProperty( property );
 
-    // Be graceful if given a Property that is not registered in an order dependency.
-    if ( this.propertyInAnOrderDependency( property ) ) {
-      assert && assert( this.propertyInAnOrderDependency( property ), 'Property must be registered in an order dependency to be unregistered' );
+      // Be graceful if given a Property that is not registered in an order dependency.
+      if ( this.propertyInAnOrderDependency( property ) ) {
+        assert && assert( this.propertyInAnOrderDependency( property ), 'Property must be registered in an order dependency to be unregistered' );
 
-      const phetioIDToRemove = property.tandem.phetioID;
+        const phetioIDToRemove = property.tandem.phetioID;
 
-      this.mapPairs.forEach( mapPair => {
-        [ mapPair.beforeMap, mapPair.afterMap ].forEach( map => {
-          if ( map.has( phetioIDToRemove ) ) {
-            map.get( phetioIDToRemove ).forEach( phetioID => {
-              const setOfAfterMapIDs = map.otherMap.get( phetioID );
-              setOfAfterMapIDs && setOfAfterMapIDs.delete( phetioIDToRemove );
+        this.mapPairs.forEach( mapPair => {
+          [ mapPair.beforeMap, mapPair.afterMap ].forEach( map => {
+            if ( map.has( phetioIDToRemove ) ) {
+              map.get( phetioIDToRemove ).forEach( phetioID => {
+                const setOfAfterMapIDs = map.otherMap.get( phetioID );
+                setOfAfterMapIDs && setOfAfterMapIDs.delete( phetioIDToRemove );
 
-              // Clear out empty entries to avoid having lots of empty Sets sitting around
-              setOfAfterMapIDs.size === 0 && map.otherMap.delete( phetioID );
-            } );
-          }
-          map.delete( phetioIDToRemove );
-        } );
-      } );
+                // Clear out empty entries to avoid having lots of empty Sets sitting around
+                setOfAfterMapIDs.size === 0 && map.otherMap.delete( phetioID );
+              } );
+            }
+            map.delete( phetioIDToRemove );
+          } );
+        } );
 
-      // Look through every dependency and make sure the phetioID to remove has been completely removed.
-      assertSlow && this.mapPairs.forEach( mapPair => {
-        [ mapPair.beforeMap, mapPair.afterMap ].forEach( map => {
-          for ( const [ key, valuePhetioIDs ] of map ) {
-            assertSlow && assertSlow( key !== phetioIDToRemove, 'should not be a key' );
-            assertSlow && assertSlow( !valuePhetioIDs.has( phetioIDToRemove ), 'should not be in a value list' );
-          }
-        } );
-      } );
+        // Look through every dependency and make sure the phetioID to remove has been completely removed.
+        assertSlow && this.mapPairs.forEach( mapPair => {
+          [ mapPair.beforeMap, mapPair.afterMap ].forEach( map => {
+            for ( const [ key, valuePhetioIDs ] of map ) {
+              assertSlow && assertSlow( key !== phetioIDToRemove, 'should not be a key' );
+              assertSlow && assertSlow( !valuePhetioIDs.has( phetioIDToRemove ), 'should not be in a value list' );
+            }
+          } );
+        } );
+      }
     }
   }
 

[samreid]

That looks great, I removed extraneous guards, tested in natural selection and everything looks good. Back to @zepumph for double check, then I think this issue can be closed.

[zepumph]

Looks great to me! I also want to note that besides PropertyStateHandler.initialize, we no-oped all of the public functions in the class in phet brand. Closing.

[pixelzoom]

Reopening. Have all sims that this affects been identified? Do they know which shas to cherry-pick? Do they have sim-specific issues?

[Denz1994]

Build a molecule is currently at 1.0.0.rc.2 as of today, is there a set of shas I should cherry-pick @zepumph? This is a phet brand sim.

[zepumph]

Right!

I think the list of sims that will need this change are:

• SOM
• EFAC
• ph-scale
• BAM

Depending on how old shas are, likely just trying to cherry-pick e52271c will not work. Instead it will need to be a manual patch in which the entire guts of two functions, PropertyStateHandler.registerPhetioOrderDependency and unregisterOrderDependenciesForProperty are wrapped in an if statement like:

if ( Tandem.PHET_IO_ENABLED ) {
 . . . 
}

To test that this fixes the leak, you will want to do something similary to phetsims/natural-selection#189 (comment). Unfortunately I don't exactly know what the memory leak will look like with older shas, since it won't use Set, and will instead use an array of OrderDependency instances. That being said, hopefully it will still look something like #328 (comment) but for a list called PropertyStateHandler.propertyOrderDependencies.

[zepumph]

I created issues in the sims that I know of that are currently in RC. @pixelzoom is there anything more that you recommend? Please review by comment above about how to cherry-pick changes. This is complicated by the large refactor done 2 weeks ago in PropertyStateHandler (memory leak was most likely present before that work also).

[samreid]

First, I'd like to check whether this sim is affected by this memory leak. Self-assigning to test it out before we proceed with a maintenance release.

[pixelzoom]

First, I'd like to check whether this sim is affected by this memory leak.

@samreid which sim are you referring to? Several have been identified.

[samreid]

If I understand correctly, EFAC SHAs will be taken from master in the near future, and on Slack it was agreed that ph-Scale would take SHAs from master for its next release. So I'll check BAM and SOM to see if they need this in a maintenance release.

[samreid]

I tested this patch with Natural Selection in phet brand and saw that it had 20,000+ listeners in a minute or two:

Index: js/PropertyStateHandler.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/PropertyStateHandler.js	(revision e52271cf099a7665dbd7f293b20dd04c3e9c8159)
+++ js/PropertyStateHandler.js	(date 1598483962306)
@@ -132,16 +132,18 @@
    * @param {PropertyStatePhase} afterPhase
    */
   registerPhetioOrderDependency( beforeProperty, beforePhase, afterProperty, afterPhase ) {
-    if ( Tandem.PHET_IO_ENABLED ) {
+    // if ( Tandem.PHET_IO_ENABLED ) {
 
-      this.validatePropertyPhasePair( beforeProperty, beforePhase );
-      this.validatePropertyPhasePair( afterProperty, afterPhase );
-      assert && beforeProperty === afterProperty && assert( beforePhase !== afterPhase, 'cannot set same Property to same phase' );
+    this.validatePropertyPhasePair( beforeProperty, beforePhase );
+    this.validatePropertyPhasePair( afterProperty, afterPhase );
+    assert && beforeProperty === afterProperty && assert( beforePhase !== afterPhase, 'cannot set same Property to same phase' );
 
-      const mapPair = this.getMapPairFromPhases( beforePhase, afterPhase );
+    const mapPair = this.getMapPairFromPhases( beforePhase, afterPhase );
 
-      mapPair.addOrderDependency( beforeProperty.tandem.phetioID, afterProperty.tandem.phetioID );
-    }
+    mapPair.addOrderDependency( beforeProperty.tandem.phetioID, afterProperty.tandem.phetioID );
+
+    // console.log();
+    // }
   }
 
   /**
@@ -160,7 +162,7 @@
    * @public
    */
   unregisterOrderDependenciesForProperty( property ) {
-    if ( Tandem.PHET_IO_ENABLED ) {
+    // if ( Tandem.PHET_IO_ENABLED ) {
       this.validateInstrumentedProperty( property );
 
       // Be graceful if given a Property that is not registered in an order dependency.
@@ -194,7 +196,7 @@
           } );
         } );
       }
-    }
+    // }
   }
 
   /**
@@ -435,12 +437,16 @@
     if ( !this.beforeMap.has( beforePhetioID ) ) {
       this.beforeMap.set( beforePhetioID, new Set() );
     }
-    this.beforeMap.get( beforePhetioID ).add( afterPhetioID );
+    const b = this.beforeMap.get( beforePhetioID );
+    b.add( afterPhetioID );
 
     if ( !this.afterMap.has( afterPhetioID ) ) {
       this.afterMap.set( afterPhetioID, new Set() );
     }
-    this.afterMap.get( afterPhetioID ).add( beforePhetioID );
+    const a = this.afterMap.get( afterPhetioID );
+    a.add( beforePhetioID );
+
+    console.log( a.size, b.size );
   }
 
   /**
Index: js/Property.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Property.js	(revision e52271cf099a7665dbd7f293b20dd04c3e9c8159)
+++ js/Property.js	(date 1598484023766)
@@ -483,7 +483,7 @@
     this.unlinkAll();
 
     // unregister any order dependencies for this Property for PhET-iO state
-    if ( this.isPhetioInstrumented() ) {
+    if ( false && this.isPhetioInstrumented() ) {
       propertyStateHandlerSingleton.unregisterOrderDependenciesForProperty( this );
     }
 

I tested the same patch in Build a Molecule fuzzing and the listeners leveled out around 2, so it did not have this same symptom. Likewise, States of Matter leveled out around 3, so it did not have this symptom.

I did not take the time to run this analysis for the RC shas, but this is likely to be a good measure of the sim behavior, since the sim-specific code probably hasn't changed much since the RCs. This is more likely a problem in Natural Selection since it uses PhetioGroup to dynamically create and destroy PhetioObject instances frequently. I'm not sure why it was not the case in Build a Molecule, are things preallocated?

In any case, it seems like those sims don't need this in a maintenance release, especially if QA testing revealed no memory leaks.

[samreid]

@pixelzoom can you please review the past 3 comments and see if this issue can be closed?

[pixelzoom]

I tested this patch with Natural Selection in phet brand and saw that it had 20,000+ listeners in a minute or two:

I don't understand this patch or what you're trying to demonstrate. Are you purposely not unregistering dependencies and counting how many there are? If so, why?

[pixelzoom]

Oh I (maybe) see. Are you purposely not unregistering dependencies so that you can see how how many a sim has, and therefore whether this fix is important to that sim?

[samreid]

The memory leak comes from items being added over and over to the sets in PropertyStateHandler. We saw this in Natural Selection a few days ago. I reverted the fix for this issue and confirmed I saw the problem again in Natural Selection (Sets in ProppertyStateHandler growing without bounds). With the fix still reverted, I tested BAM and SOM and saw the lists never went beyond 2 or 3 during at least minute of fuzzing. Therefore they do not exercise PropertyStateHandler in a way that produces a memory leak. Therefore they do not require this patch for correct operation as they are now.

If we think it is likely that a maintenance branch of SOM or BAM will start triggering this scenario (I believe it's by disposing numerous instrumented Properties that have order dependencies), then we should apply the patch, following the instructions in #328 (comment). To me, it seems unlikely to create a maintenance release that triggers this bug, so it may not be worth the effort. I ran this test not knowing what answer I would get. If I saw a memory leak, it would have been easy to say "it's essential to maintenance release this", but since there was no leak, it probably doesn't need the patch. But the patch wouldn't hurt and wouldn't take long to apply. I don't feel strongly one way or another, just wanted to identify options, costs and risks.

UPDATE: Two other angles I considered that I think I should make explicit:

• I'm starting to get concerned about "cherry pick fatigue"--hearing about numerous branches having to cherry pick more and more things seems problematic, and I am worried this is suboptimal. That is why I wanted to see if this cherry pick (which is likely to be non-automatic) is necessary.
• I wonder if we should be using the automated maintenance release process to automatically identify which repos need to be fixed, instead of trying to manually do so. (But maybe it would only be semi-automatic anyways if we have to identify the first SHA where the problem was introduced?)

[pixelzoom]

In a related PhET-iO issue, #328 (comment), @samreid said:

I'm starting to get concerned about "cherry pick fatigue" ...

Yes, that has been a concern (and recurring cost) for me since mid-July, when QA first started reporting problems for ph-scale 1.4.0-rc.1. For previous sims, it's been a non-issue. There are usually a handful of sim and common-code issues that need to be picked up in the branch, and it has been straightforward. But cherry-picking is only one of many problems that I encountered, which you can read more about in the postmortem at https://github.com/phetsims/special-ops/issues/165.

[pixelzoom]

Memory tests look good for both NS brands, see phetsims/natural-selection#189 and phetsims/natural-selection#190.

Feel free to close if there's nothing else to do here.