Memory leak test and fix

[samreid]

We should self-test memory leaks before giving to QA

[zepumph]

Running one now for PhET-iO standalone, capturing every few minutes.

[zepumph]

Oh boy, not looking good:

1: startup no fuzzing or interaction 2: fuzzing after a few seconds 6: fuzzing after 6 or so minutes

[zepumph]

Easiest way to work this problem:

• Load sim without fuzzing and take a snapshot
• reload with fuzzing and let er rip for a couple seconds
• in the console type: phet.chipper.queryParameters.fuzz = false;
• manually press reset all on both screens
• take another snapshot
• compare to first snapshot

[zepumph]

@samreid and I worked on this and found great success with a couple of leaks.

We then realized that tandems (especially DynamicTandem) are not being disposed in PhET brand. Uh oh. This is because tandem disposal is triggered only if the phetioObject has been "initialized", and then only after passing that to tandem.removePhetioObject (why?!!?). Anywho, it is definitely a memory leak, but only in PhET brand and only for dynamic tandems (since all others exist forever anyways).

[samreid]

I observed that phetioObjectInitialized is indeed false in phet brand. Then I changed this code, and the DynamicTandem leaks disappeared from phet brand:

Subject: [PATCH] Improve selection highlighting, see https://github.com/phetsims/circuit-construction-kit-common/issues/975
---
Index: js/PhetioObject.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/PhetioObject.ts b/js/PhetioObject.ts
--- a/js/PhetioObject.ts    (revision d735eab635e4d72a6db67f02ae69a6389e6f3c45)
+++ b/js/PhetioObject.ts    (date 1677114323415)
@@ -657,9 +657,7 @@
       } );
     } );

-    if ( this.phetioObjectInitialized ) {
-      this.tandem.removePhetioObject( this );
-    }
+    this.tandem.removePhetioObject( this );

     // Dispose LinkedElements
     if ( this.linkedElements ) {

@zepumph does that seem reasonable to commit?

[samreid]

I think I'll commit that since it addresses a known memory leak in phet brand, and since I want to continue memory testing with that change.

[samreid]

After those fixes, I think the memory leak is probably addressed. Maybe we could use a bit more testing to double check.

[samreid]

After the fixes, I ran a memory snapshot every 5 minutes (first one after 5 minutes), by toggling fuzzing via phet.chipper.queryParameters.fuzz = !phet.chipper.queryParameters.fuzz;. After 20 minutes, there is 0.4MB over that 5 minute period and it looks like noise. So this seems we are in good shape, and probably need to check in with @zepumph about review for https://github.com/phetsims/circuit-construction-kit-common/issues/977#issuecomment-1441097513. I also considered the recommendation in https://github.com/phetsims/scenery/issues/1494#issuecomment-1400612730 about inlining disposals--it would have made these memory fixes easier to implement and may have avoided some of the leaks in the first place.

[matthew-blackman]

I did some testing over the last hour and found the heap snapshots leveling off at around 90MB after about 30-40 mins. This seems consistent with the object pools filling.

[zepumph]

RE: https://github.com/phetsims/circuit-construction-kit-common/issues/977#issuecomment-1441097513.

This deserves a code comment. And I feel good about it as long as you have memory tested phet and phet-io brand and all looks good. Tandem.removePhetioObject is already quite defensive, so it is pretty low risk in my book.

[samreid]

Thanks. I added documentation as to my understanding of that part. I feel confident in closing the issue, but @zepumph or anyone please reopen if there's more to do or discuss.