phetsims / tandem

Simulation-side code for PhET-iO
MIT License
0 stars 5 forks source link

Dispose tandems when disposing phetioGroup Element #264

Closed marlitas closed 2 years ago

marlitas commented 2 years ago

While working on Mean: Share and Balance @samreid and I noticed a memory leak that involved tandem and dynamicTandem.

To recreate:

  1. Launch Mean: Share and Balance
  2. Warm up Sim, go up to 7 cups then back down to 1
  3. Take a memory snapshot
  4. Go up to 7 cups then back down to 1
  5. Take another memory snapshot
  6. Compare snapshots and organize by deltas
  7. Tandem and dynamicTandem had a +delta that was divisible by 6 (the amount of cups added and removed).

Upon further investigation we added element.tandem.dispose(); to PhetioDynamicElementContainer.ts line 355 ( inside disposeElement function). This eliminated the tandem leak.

Before fully implementing this change we wanted to check with @zepumph to make sure this wouldn't waterfall into affecting other phetio functionality negatively.

zepumph commented 2 years ago

Great catch @marlitas! Let me do some explaining.

Tandems are already set up to be "monolithic" in the sense that only one instance of Tandem should exist for a given phetioID. Thus, if you createTandem to make something that already exists, createTandem() will just return that instance instead of creating a duplicate instance. See here:

https://github.com/phetsims/tandem/blob/ddd87c95802b465c50ed570e4e8cc5f46a863191/js/Tandem.js#L246-L256

For dynamic elements, we do the same logic, where we re-use tandems if available (most often they aren't though, like you in your case where they have already been disposed, but we'll get to that):

https://github.com/phetsims/tandem/blob/0dc0a129ef3f9db8644960bec652cb40b3671a9f/js/PhetioDynamicElementContainer.ts#L266-L272

So much of my investigation here was confirming that DynamicTandems are supported in that same way, and shouldn't just be re-created each time a new element is made. The same goes for disposal. We don't necessarily need to dispose it manually for dynamic elements, because it should be disposed automatically if it has no more children etc. The issue was that when disposing the child, we were cutting the parent off from the child, but not the child from the parent. Thus the child, though disposed, still had a reference to the parent, and thus wouldn't get garbage collected. I believe this solves you leak. Please review.

zepumph commented 2 years ago

Most likely this changed is causing https://github.com/phetsims/natural-selection/issues/311#event-6954427973. I think we have to do a better job of setting the parentTandem once using that instance again.

zepumph commented 2 years ago

Just a small extra step in making sure we don't do API validation on disposed elements.

marlitas commented 2 years ago

This got fixed when we moved to static allocation: https://github.com/phetsims/mean-share-and-balance/issues/60 closing.