Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General disposal problems #434

Closed
zepumph opened this issue Apr 4, 2023 · 4 comments
Closed

General disposal problems #434

zepumph opened this issue Apr 4, 2023 · 4 comments

Comments

@zepumph
Copy link
Member

zepumph commented Apr 4, 2023

While working on #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 added a commit to phetsims/scenery that referenced this issue Apr 4, 2023
@zepumph
Copy link
Member Author

zepumph commented Apr 4, 2023

@samreid can you please double check phetsims/tandem@4dd4290

@jonathanolson can you please double check phetsims/scenery@0a6ba41

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

@jonathanolson
Copy link
Contributor

phetsims/scenery@0a6ba41 looks good!

@jonathanolson jonathanolson removed their assignment Apr 4, 2023
@samreid
Copy link
Member

samreid commented Apr 5, 2023

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

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.

@samreid samreid assigned zepumph and unassigned samreid Apr 5, 2023
zepumph added a commit to phetsims/tandem that referenced this issue Apr 5, 2023
@zepumph
Copy link
Member Author

zepumph commented Apr 5, 2023

Righto thanks. Closing

@zepumph zepumph closed this as completed Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants