Eagerly create the KeyboardHelpDialog?

[zepumph]

We decided to use the PhetioCapsule strategy for dialogs because we say a noticeable difference in memory when creating the AboutDialog eagerly. This creates a lot of Nodes and content. But I was surprised that KeyboardHelpDialog content is not lazily created:

https://github.com/phetsims/ratio-and-proportion/blob/d4f186a42f9bc72f3cca525ea61891dc62334678/js/discover/DiscoverScreen.ts#L29

(for example)

So why don't we just save the complexity of PhetioCapsule if we don't even cover creating the content dynamically? The alternative would be to lazily create the content too. I'm happy with either, but it may be complicated by disposing per-screen content.

@samreid what do you think? Do you remember our investigation with AboutDialog?

[samreid]

Yes, and I'm surprised to see this eager creation. I recall AboutDialog took time and memory to create (but I don't recall those numbers). I suspect we will want to convert to the pattern from https://github.com/phetsims/sun/issues/746 and convert:

keyboardHelpNode?: Node | null;

to

keyboardHelpNode?: (tandem: Tandem) => Node;

Does that sound right to you? But I'm also fine to create all AboutDialog/PreferencesDialog/KeyboardHelpDialog up front as long as it can be done < 150ms.

[zepumph]

I am happy to take the AboutDialog findings that live in my brain from 2+ years ago as gospel (though we could retest things). With that in mind, KeyboardHelpDialog on average creates many more Nodes and view components, so I definitely think that your understanding is correct, we should dynamically create these to save on startup, and also begin disposing this stuff.

I would like to take a first pass at implementing because we have screen-specific content, and so each may need particular logic to be lazily created.

[zepumph]

• [ ] Next here we will want to deal with disposal. This is a very large memory leak at this time.

[zepumph]

• [x] Don't forget about https://github.com/phetsims/joist/issues/876

[zepumph]

• [x] @jessegreenberg, should we assert that if any sim screen has a createKeyboardHelpNode function, then every single screen created in the runtime also has to? I think I made it a bit too graceful yesterday in my commits.

[jessegreenberg]

Yes since we won't have a single option in the Sim anymore that would be helpful. Had a slight concern about the dev time impact. Instead of adding it to a screen, then seeing what it looks like we will need to add boilerplate to each screen before we can test. Its worth it in my opinion.

[zepumph]

Sounds good. Done above.

[zepumph]

Disposal is covered over in https://github.com/phetsims/scenery-phet/issues/769. Closing

[zepumph]

With these closures + creating content per screen, we increased the memory of the keyboard help content by a lot. https://github.com/phetsims/joist/issues/890. I wonder if we can reuse components between screens.

[zepumph]

With this patch we save 7MB on the heap when opening the keyboardHelpDialog,

Index: js/GOSim.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/GOSim.ts b/js/GOSim.ts
--- a/js/GOSim.ts   (revision 18b06617c2aee565248c542e56af4a9f8b776c3e)
+++ b/js/GOSim.ts   (date 1670448297034)
@@ -18,6 +18,8 @@
 import PreferencesModel from '../../joist/js/preferences/PreferencesModel.js';
 import GOPreferences from './common/model/GOPreferences.js';
 import TReadOnlyProperty from '../../axon/js/TReadOnlyProperty.js';
+import { Node } from '../../scenery/js/imports.js';
+import GOKeyboardHelpContent from './common/view/GOKeyboardHelpContent.js';

 type SelfOptions = {

@@ -54,13 +56,24 @@
       } )
     }, providedOptions );

+    let keyboardHelp: null | Node;
+
+    const createKeyboardHelpNode = () => {
+      if ( !keyboardHelp ) {
+        keyboardHelp = new GOKeyboardHelpContent();
+      }
+      return keyboardHelp;
+    };
+
     super( titleProperty, [
       new LensScreen( {
         isBasicsVersion: options.isBasicsVersion,
+        createKeyboardHelpNode: createKeyboardHelpNode,
         tandem: Tandem.ROOT.createTandem( 'lensScreen' )
       } ),
       new MirrorScreen( {
         isBasicsVersion: options.isBasicsVersion,
+        createKeyboardHelpNode: createKeyboardHelpNode,
         tandem: Tandem.ROOT.createTandem( 'mirrorScreen' )
       } )
     ], options );
Index: js/lens/LensScreen.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/lens/LensScreen.ts b/js/lens/LensScreen.ts
--- a/js/lens/LensScreen.ts (revision 18b06617c2aee565248c542e56af4a9f8b776c3e)
+++ b/js/lens/LensScreen.ts (date 1670448297057)
@@ -10,7 +10,6 @@
 import Screen, { ScreenOptions } from '../../../joist/js/Screen.js';
 import ScreenIcon from '../../../joist/js/ScreenIcon.js';
 import GOColors from '../common/GOColors.js';
-import GOKeyboardHelpContent from '../common/view/GOKeyboardHelpContent.js';
 import geometricOptics from '../geometricOptics.js';
 import GeometricOpticsStrings from '../GeometricOpticsStrings.js';
 import LensModel from './model/LensModel.js';
@@ -22,7 +21,7 @@

 type SelfOptions = PickRequired<GOSimOptions, 'isBasicsVersion'>;

-type LensScreenOptions = SelfOptions & PickRequired<ScreenOptions, 'tandem'>;
+type LensScreenOptions = SelfOptions & PickRequired<ScreenOptions, 'tandem' | 'createKeyboardHelpNode'>;

 export default class LensScreen extends Screen<LensModel, LensScreenView> {

@@ -33,8 +32,7 @@
       // Screen options
       name: GeometricOpticsStrings.screen.lensStringProperty,
       homeScreenIcon: createScreenIcon(),
-      backgroundColorProperty: GOColors.screenBackgroundColorProperty,
-      createKeyboardHelpNode: () => new GOKeyboardHelpContent()
+      backgroundColorProperty: GOColors.screenBackgroundColorProperty
     }, providedOptions );

     super(
Index: js/mirror/MirrorScreen.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/mirror/MirrorScreen.ts b/js/mirror/MirrorScreen.ts
--- a/js/mirror/MirrorScreen.ts (revision 18b06617c2aee565248c542e56af4a9f8b776c3e)
+++ b/js/mirror/MirrorScreen.ts (date 1670448297046)
@@ -15,7 +15,6 @@
 import MirrorModel from './model/MirrorModel.js';
 import MirrorNode from './view/MirrorNode.js';
 import MirrorScreenView from './view/MirrorScreenView.js';
-import GOKeyboardHelpContent from '../common/view/GOKeyboardHelpContent.js';
 import { OpticSurfaceType } from '../common/model/OpticSurfaceType.js';
 import optionize from '../../../phet-core/js/optionize.js';
 import PickRequired from '../../../phet-core/js/types/PickRequired.js';
@@ -23,7 +22,7 @@

 type SelfOptions = PickRequired<GOSimOptions, 'isBasicsVersion'>;

-type MirrorScreenOptions = SelfOptions & PickRequired<ScreenOptions, 'tandem'>;
+type MirrorScreenOptions = SelfOptions & PickRequired<ScreenOptions, 'tandem' | 'createKeyboardHelpNode'>;

 export default class MirrorScreen extends Screen<MirrorModel, MirrorScreenView> {

@@ -34,8 +33,7 @@
       // Screen options
       name: GeometricOpticsStrings.screen.mirrorStringProperty,
       homeScreenIcon: createScreenIcon( providedOptions.isBasicsVersion ? 'flat' : 'concave' ),
-      backgroundColorProperty: GOColors.screenBackgroundColorProperty,
-      createKeyboardHelpNode: () => new GOKeyboardHelpContent()
+      backgroundColorProperty: GOColors.screenBackgroundColorProperty
     }, providedOptions );

     super(

[zepumph]

@jessegreenberg can we brainstorm on if there is a better way to support screen-specific content here?

[zepumph]

I think that in general, we should use sim-specific code to reuse keyboard help content between screens, when applicable. The above patch is simple and straightforward, and helps keep the keyboard dialog flexible to having different content per screen (like on the home screen of most sims). I expect that alt input will continue to use screen-specific content as we progress the feature (like in RAP). I'll go through some sims and clean up memory.

[zepumph]

Alright, all other sims are either a single screen sim, or are using different help contents per screen. Closing