memory leak in Tandem.removePhetioObject

[pixelzoom]

Over in phetsims/natural-selection#189 (comment), I identified a huge memory leak in Natural Selection due to PhET-iO, even with brand=phet. I determined that 0 instances of Tandem and DynamicTandem were being deleted.

@samreid helped me identify the source of the problem. The leak was introduced on 5/1/2020 in 571cc87 for #176. It affects all sims that have been published since then.

@samreid provided the patch below, which I confirmed address the leaking of Tandem and DynamicTandem in natural-selection. This patch needs to be reviewed by @zepumph, with top priority.

patch

Index: js/Tandem.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/Tandem.js	(revision fbd3b1c92b1d25369476ea270402326632ef632e)
+++ js/Tandem.js	(date 1598374832259)
@@ -202,12 +202,13 @@
         arrayRemove( bufferedPhetioObjects, phetioObject );
       }
       else {
-        phetioObject.tandem.dispose();
+
         for ( let i = 0; i < phetioObjectListeners.length; i++ ) {
           phetioObjectListeners[ i ].removePhetioObject( phetioObject );
         }
       }
     }
+    phetioObject.tandem.dispose();
   }
   /**

Then we need to identify which sims have been published with this problem, decide which to patch, schedule maintenance releases, etc.

Assigning to @jbphet because this leak is almost certainly present in the recently-published SOM.

[pixelzoom]

This is blocking ph-scale RC, see phetsims/ph-scale#199.

[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".

[samreid]

The patch disposes of the Tandem in every brand, not just in phet-io. It looks correct to me, leaving assigned to @zepumph to confirm.

[samreid]

I decided to commit the proposal, still ready for review.

[zepumph]

This change looks good to me. I would also add that this is better to dispose after calling phetioObjectRemoved listeners because those listeners may depend on the tandem (like phetioAPIValidation.onPhetioObjectRemoved). Good catch and thanks for the implementation. I did not test to confirm that it fixed the memory leak. I only looked at the code.

[zepumph]

Any rc taken after 5/1 would be effected by this. I know that means at least:

• ph-scale
• SOM

@Denz1994 I'm unsure about BaM
@chrisklus, I'm unsure about EFAC

This fix should be very simple though, just cherrypick dc10b03. This issue effects brand=phet.

[zepumph]

Assigning to @Denz1994 and @chrisklus to determine if their RC was made after 5/1. If so to apply the patch, either in this issue or a sim-specific one.

The patch is dc10b03.

[zepumph]

Any RC with shas after 5/1 has a memory leak in phet brand in #203. I know of only what I spoke of above. Assigning to @ariel-phet to make sure that this doesn't effect other sims published. Likely there are prototypes that effect this, but maybe we don't care about memory leaks in those.

[samreid]

Shouldn't we use the batch maintenance release process to automatically identify all live branches with this bug and automatically or semi-automatically fix them? (This question goes for all bugs fixed during any RC).

[pixelzoom]

Is this issue ready for verification in Natural Selection, so that I can resolve phetsims/natural-selection#205 and move forward with prototype publication?

[samreid]

Yes, please pull and test to confirm this is ready for Natural Selection. This has been fixed and reviewed in master. The only reason this issue remains open is to see what branches it may need to be cherry-picked to.

[pixelzoom]

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

[pixelzoom]

Oops. Reopening because @samreid said:

The only reason this issue remains open is to see what branches it may need to be cherry-picked to.

[zepumph]

Agreed. EFAC will be taken (was taken yesterday?) from master. I think all that is left is to do is for @Denz1994 to patch BAM if needed.

[Denz1994]

Thanks, @zepumph for the direction. This was patched in BAM. Removing my assignment.

[zepumph]

Sounds good. I don't any other sims are in RC right now. Over to @ariel-phet to confirm and close

[ariel-phet]

I think we are good here, closing