phetsims / calculus-grapher

"Calculus Grapher" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

Patch memory leak in Disposable. #330

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

For https://github.com/phetsims/qa/issues/924

While reviewing https://github.com/phetsims/scenery-phet/issues/769, @zepumph and I discovered a relatively serious memory leak in Disposable, which is the base class for (among other things) PhetioObject.

From https://github.com/phetsims/scenery-phet/issues/769#issuecomment-1495060141, the line added in this diff is missing, so disposeEmitter listeners are not freed.

  public dispose(): void {
    assert && assert( !this.isDisposed, 'Disposable can only be disposed once' );
    this._disposeEmitter.emit();
+   this._disposeEmitter.dispose();
    this.isDisposed = true;
  }

In calculus-grapher, we use disposeEmitter througout GraphTypeLabelNode, which appears as labels for the Notation radio buttons appears in the Preferences dialog: f'(x), f'(t), df/dx, df/dt. An example from GraphTypeLabelNode is:

  label.disposeEmitter.addListener( () => {
    labelStringProperty.dispose();
  } );

In Slack#DM, I asked @zepumph about the above example:

There’s no handle to the listener function, butlabelStringProperty is included by closure. Is this going to be a leak? Should we patch in calculus-grapher 1.0 ?

... and he replied:

Patching the Branch seems prudent.

So I'm going to proceed with patching calculus-grapher 1.0, to be tested in the next RC.

@veillette @amanda-phet FYI.

zepumph commented 1 year ago

The commit to cherry pick is https://github.com/phetsims/axon/commit/022433795034acf606ba510330a7495d1d56702a

pixelzoom commented 1 year ago

Cherry-picked and patches into axon's 'calculus-grapher-1.0' branch.

To verify in the next RC, repeat memory testing and compare to the results for the previous RC that are shown in https://github.com/phetsims/qa/issues/924#issuecomment-1487547110.

KatieWoe commented 1 year ago

Memory looks ok to me.

cgmem