-
Notifications
You must be signed in to change notification settings - Fork 7
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
Implement dispose for keyboard-help content #253
Comments
There are plans for a PhET-iO release of this sim, so self assigning. Blocked until phetsims/scenery-phet#769 is addressed. |
Note that we decided to punt on alt input and keyboard help for the 1.6 release. See #249. Also note that |
In the above commit, I implemented
I suspect the problem is somewhere in scenery-phet/js/keyboard. @zepumph FYI. |
I ran this test again with the same code, and got drastically different results:
@zepumph any idea what could account for this? And any advice for tracking down the memory leak here? |
I'll take a look for a few minutes. In general I do a comparison view in the chrome memory tab, and search for "Keyboard" to see if there are dangling keyboard-help-specific stuff. |
Hmmm, to me it looks like it may have been fixed already, or a bug in chrome, I got:
Can you retest on your side? Here is my patch Subject: [PATCH] formatting
---
Index: ph-scale/js/macro/MacroScreen.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ph-scale/js/macro/MacroScreen.ts b/ph-scale/js/macro/MacroScreen.ts
--- a/ph-scale/js/macro/MacroScreen.ts (revision 5f8363908d78827d2c992f61853b5b680a0609c1)
+++ b/ph-scale/js/macro/MacroScreen.ts (date 1680709152079)
@@ -20,6 +20,7 @@
import PhScaleStrings from '../PhScaleStrings.js';
import MacroModel from './model/MacroModel.js';
import MacroScreenView from './view/MacroScreenView.js';
+import MacroKeyboardHelpContent from './view/MacroKeyboardHelpContent.js';
type SelfOptions = EmptySelfOptions;
@@ -41,9 +42,9 @@
navigationBarIcon: new ScreenIcon( new Image( macroNavbarIcon_png ), {
maxIconWidthProportion: 1,
maxIconHeightProportion: 1
- } )
+ } ),
//TODO https://github.com/phetsims/ph-scale/issues/249 restore when work on alternative input resume
- // createKeyboardHelpNode: () => new MacroKeyboardHelpContent()
+ createKeyboardHelpNode: () => new MacroKeyboardHelpContent()
}, providedOptions );
super(
Index: ph-scale/js/mysolution/MySolutionScreen.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ph-scale/js/mysolution/MySolutionScreen.ts b/ph-scale/js/mysolution/MySolutionScreen.ts
--- a/ph-scale/js/mysolution/MySolutionScreen.ts (revision 5f8363908d78827d2c992f61853b5b680a0609c1)
+++ b/ph-scale/js/mysolution/MySolutionScreen.ts (date 1680709152082)
@@ -20,6 +20,7 @@
import PhScaleStrings from '../PhScaleStrings.js';
import MySolutionModel from './model/MySolutionModel.js';
import MySolutionScreenView from './view/MySolutionScreenView.js';
+import MySolutionKeyboardHelpContent from './view/MySolutionKeyboardHelpContent.js';
type SelfOptions = EmptySelfOptions;
@@ -41,9 +42,9 @@
navigationBarIcon: new ScreenIcon( new Image( mySolutionNavbarIcon_png ), {
maxIconWidthProportion: 1,
maxIconHeightProportion: 1
- } )
+ } ),
//TODO https://github.com/phetsims/ph-scale/issues/249 restore when work on alternative input resume
- // createKeyboardHelpNode: () => new MySolutionKeyboardHelpContent()
+ createKeyboardHelpNode: () => new MySolutionKeyboardHelpContent()
}, providedOptions );
super(
Index: ph-scale/js/micro/MicroScreen.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ph-scale/js/micro/MicroScreen.ts b/ph-scale/js/micro/MicroScreen.ts
--- a/ph-scale/js/micro/MicroScreen.ts (revision 5f8363908d78827d2c992f61853b5b680a0609c1)
+++ b/ph-scale/js/micro/MicroScreen.ts (date 1680709152085)
@@ -20,6 +20,7 @@
import MicroModel from './model/MicroModel.js';
import MicroScreenView from './view/MicroScreenView.js';
import PickRequired from '../../../phet-core/js/types/PickRequired.js';
+import MicroKeyboardHelpContent from './view/MicroKeyboardHelpContent.js';
type SelfOptions = EmptySelfOptions;
@@ -41,9 +42,9 @@
navigationBarIcon: new ScreenIcon( new Image( microNavbarIcon_png ), {
maxIconWidthProportion: 1,
maxIconHeightProportion: 1
- } )
+ } ),
//TODO https://github.com/phetsims/ph-scale/issues/249 restore when work on alternative input resume
- // createKeyboardHelpNode: () => new MicroKeyboardHelpContent()
+ createKeyboardHelpNode: () => new MicroKeyboardHelpContent()
}, providedOptions );
super(
Index: joist/js/KeyboardHelpButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/joist/js/KeyboardHelpButton.ts b/joist/js/KeyboardHelpButton.ts
--- a/joist/js/KeyboardHelpButton.ts (revision ef0f5216d231b5994912c40a742f35f37b9d257f)
+++ b/joist/js/KeyboardHelpButton.ts (date 1680709838500)
@@ -72,7 +72,7 @@
super( icon, backgroundColorProperty, options );
keyboardHelpDialogCapsule = new PhetioCapsule<KeyboardHelpDialog>( tandem => {
-
+ console.log( 'new keyboardDialog' );
// Wrap in a node to prevent DAG problems if archetypes are also created
return new KeyboardHelpDialog( screens, screenProperty, {
tandem: tandem,
@@ -87,6 +87,11 @@
backgroundColorProperty.link( backgroundColor => {
icon.image = backgroundColor.equals( Color.BLACK ) ? keyboardIcon_png : keyboardIconOnWhite_png;
} );
+
+ for ( let i = 0; i < 10; i++ ) {
+ keyboardHelpDialogCapsule.getElement();
+ keyboardHelpDialogCapsule.disposeElement();
+ }
}
}
|
Again with an incognito window, I see a snapshot of 50MB with 100 cycles. |
Confirmed, I'm seeing 50MB heap size with 1, 10, and 100 instances of KeyboardHelpDialog. I'll proceed with commiting changes that are specific to ph-scale. |
I restore Note that the keyboard button will not appear in the navigation bar until #249 is addressed. Closing. |
Related to phetsims/scenery-phet#769 ...
This sim currently does not dispose of keyboard-help content, and currently has
dispose
implemented like this:The sim will therefore fail in the PhET-iO State Wrapper, because it creates and then immediately disposes PhetioCapsules on startup.
Implement
dispose
for these classes:The text was updated successfully, but these errors were encountered: