phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
10 stars 8 forks source link

General disposal problems #434

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

While working on https://github.com/phetsims/axon/issues/433, I found 2 issues that @jonathanolson helped me sort out, but I want review for them so I'll make a separate issue.

  1. It is possible that unclean items freed to a pool can hold memory leaks. We must remain vigilant with cleaning before freeing.
  2. ?listenerLimit helped me realize that we are creating a callback for every single PhetioObject when disposing to runOnNextTick. Let's be better there:

https://github.com/phetsims/tandem/blob/c55de9187e936d5cdfeb4aa26be7879417019ad3/js/PhetioObject.ts#L649-L659

zepumph commented 1 year ago

@samreid can you please double check https://github.com/phetsims/tandem/commit/4dd42905a7434da317a6ccf8b518c393eb69fac4

@jonathanolson can you please double check https://github.com/phetsims/scenery/commit/0a6ba413d5413bf4fe4e352c7110ea1377e83ded

feel free to unassign and last one here feel free to close.

jonathanolson commented 1 year ago

https://github.com/phetsims/scenery/commit/0a6ba413d5413bf4fe4e352c7110ea1377e83ded looks good!

samreid commented 1 year ago

The changes look good, and I would recommend combining those if statements like this:

```diff Subject: [PATCH] Add modelToViewDeltaXY, see https://github.com/phetsims/build-a-nucleus/issues/73 --- 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 cbc75220f023f8ead53445ada8da532c511d0e06) +++ b/js/PhetioObject.ts (date 1680724154797) @@ -628,25 +628,24 @@ /** * Remove this phetioObject from PhET-iO. After disposal, this object is no longer interoperable. Also release any * other references created during its lifetime. + * + * In order to support the structured data stream, PhetioObjects must end the messages in the correct + * sequence, without being interrupted by dispose() calls. Therefore, we do not clear out any of the state + * related to the endEvent. Note this means it is acceptable (and expected) for endEvent() to be called on + * disposed PhetioObjects. */ public override dispose(): void { - const descendants: PhetioObject[] = []; - if ( assert && Tandem.PHET_IO_ENABLED && this.tandem.supplied ) { - const phetioEngine = phet.phetio.phetioEngine; + + // The phetioEvent stack should resolve by the next frame, so that's when we check it. + if ( assert && Tandem.PHET_IO_ENABLED && this.tandem.supplied ) { + + const descendants: PhetioObject[] = []; this.tandem.iterateDescendants( tandem => { - if ( phetioEngine.hasPhetioObject( tandem.phetioID ) ) { - descendants.push( phetioEngine.getPhetioObject( tandem.phetioID ) ); + if ( phet.phetio.phetioEngine.hasPhetioObject( tandem.phetioID ) ) { + descendants.push( phet.phetio.phetioEngine.getPhetioObject( tandem.phetioID ) ); } } ); - } - // In order to support the structured data stream, PhetioObjects must end the messages in the correct - // sequence, without being interrupted by dispose() calls. Therefore, we do not clear out any of the state - // related to the endEvent. Note this means it is acceptable (and expected) for endEvent() to be called on - // disposed PhetioObjects. - // - // The phetioEvent stack should resolve by the next frame, so that's when we check it. - if ( assert && Tandem.PHET_IO_ENABLED && this.tandem.supplied ) { animationFrameTimer.runOnNextTick( () => { // Uninstrumented PhetioObjects don't have a phetioMessageStack attribute. ```

Feel free to commit and close if that seems good to you.

zepumph commented 1 year ago

Righto thanks. Closing