Dispose tandems when disposing phetioGroup Element

[marlitas]

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]

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:

tandem/js/Tandem.js

Lines 246 to 256 in ddd87c9

	// re-use the child if it already exists, but make sure it behaves the same.
	if ( this.hasChild( name ) ) {
	const currentChild = this.children[ name ];
	assert && assert( currentChild.required === options.required );
	assert && assert( currentChild.supplied === options.supplied );
	assert && assert( currentChild instanceof Tandem );
	return currentChild;
	}
	else {
	return new Tandem( this, name, options );
	}

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):

tandem/js/PhetioDynamicElementContainer.ts

Lines 266 to 272 in 0dc0a12

	if ( !this.tandem.hasChild( componentName ) ) {
	createdObjectTandem = new DynamicTandem( this.tandem, componentName, this.tandem.getExtendedOptions() );
	}
	else {
	createdObjectTandem = this.tandem.createTandem( componentName, this.tandem.getExtendedOptions() );
	assert && assert( createdObjectTandem instanceof DynamicTandem, 'createdObjectTandem should be an instance of DynamicTandem' );
	}

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]

Most likely this changed is causing phetsims/natural-selection#311 (comment). I think we have to do a better job of setting the parentTandem once using that instance again.

[zepumph]

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

[marlitas]

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