Prevent crashing on iPad 2

[samreid]

The simulation shouldn't crash on iPad 2. I'll investigate how frequently it crashes and whether there is a way to crash less.

[samreid]

@jessegreenberg has been investigating Energy Skate Park: Basics crashing on iPad 2 in this issue: phetsims/energy-skate-park-basics#435

[samreid]

Launching built version on iPad2:
opens without crashing after about 15 seconds.
Stay on homescreen without crashing for 10 seconds.
Click into waves screen 1 and run waves for 10 seconds.
Switch to interference screen and run waves for 10 seconds
Switch to slits screen and run waves for 10 seconds
Switch to diffraction screen and scrub sliders for 10 seconds
No crash yet.

Launching built version on iPad2 with ?fuzzMouse
Crash at 48 seconds after launch

Launching built version on iPad2 with ?fuzzMouse (2nd run)
Crash at 90 seconds after launch

(3rd run)
Crash at 130 seconds after launch

[samreid]

Several minutes of fuzz testing on Mac Chrome does not reveal a memory leak:

[samreid]

I investigated crashing on Energy Skate Park: Basics in phetsims/energy-skate-park-basics#435 (comment) to see if I could find out what the crashing correlates with. But it is very difficult to discern correlations or how we could reduce the problem. Is the behavior acceptable for now? I also have asked a question in phetsims/energy-skate-park-basics#435 (comment) about whether the crashing pattern is the same in the app as it is in mobile safari.

[arouinfar]

@samreid let's wait on this issue until we get some data from dev testing. If QA finds that the sim is crashing all the time on iPad2 to the point it's unusable, we may need to do a bit more digging here. Then again, if we truly are dropping iPad2 support, then perhaps we leave things as-is. Either way, we can discuss things more after QA spends some time testing on iPad2.

[samreid]

Sounds good, thanks. I'll try to label accordingly.

[samreid]

In the aforementioned issue, we switched from hundreds of shaded sphere nodes to using one canvas. It likely impacts the memory usage, so I should run more crash tests.

[samreid]

After the SoundParticleLayer canvas, 93 seconds until fuzzMouse crash on built.

[samreid]

On hold until dev testing.

[samreid]

Launching a built version with ?fuzz on iPad air is still showing no problems after 11 minutes. That seems good enough to close this issue. We can reopen or create supplemental issues if related problems are discovered during dev or RC testing.

[samreid]

Oops, after closing I realized this issue is about iPad 2, not iPad air. Leaving open for further investigation.

[samreid]

More than 5 minutes of fuzzing on a built version on iPad 2 with no issues (except the fuzzing is notably sluggish). This may have been helped by recent PhET-iO IO type memory fixes, or omitting the fourth screen. In either case, this seems good enough to close this issue, and we can reopen if other problems are found in subsequent dev or RC testing.

[samreid]

Testing dev.51 on iPad2 today I observed a crash. I subsequently restarted safari, cleared other apps and was able to tap through every screen and every scene and use the simulation for >3 minutes with no crashing. But this sim uses around 60MB of memory, so it could be susceptible to crashing on iPad2. I'll see how far fuzzing goes at the moment.

Initial test ran 15 seconds on iPad2 fuzzing before crash.
Second test crashed at 11 minutes 38 seconds.

My hypothesis is that Safari already had memory usage when the first test began. Then when safari crashed and restarted, that was cleared. Then in the second test, Wave Interference got to use the full memory and stayed below the crashing threshold for quite some time.

Looking around in the Wave Interference heap, I don't see any obvious and easy ways to reduce memory. There are large amounts of Emitters, BooleanProperties and Bounds instances, but many of these are created by common code.

I tried throwing away 6 of the 9 CanvasNodes, but this did not yield a measurable difference in heap size (it was 66.4MB either way).

Reducing the lattice size from 100 cells squared to 50 cells squared reduces memory usage to 64MB, not a significant savings.

[samreid]

I can shave off 2.4MB (from 59.5MB to 57.1MB) in requirejs mode with assertions disabled by only declaring PhetioObject properties if PHET_IO_ENABLED. This is pretty hackish though because some of the properties such as tandem and linkedElements, need to be defined anyways, so the solution looks haphazard.

Index: tandem/js/PhetioObject.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- tandem/js/PhetioObject.js	(revision c7a56e2e760ab2e8cba1f515916b08fdb69e35e8)
+++ tandem/js/PhetioObject.js	(date 1544591922000)
@@ -66,57 +66,61 @@
     // @public (read-only) {Tandem} - assigned in initializePhetioObject - the unique tandem for this instance
     this.tandem = null;
 
-    // @public (read-only) {IOType} - assigned in initializePhetioObject - the IO type associated with this instance
-    this.phetioType = null;
+    // @private {LinkedElement[]} - keep track of LinkedElements for disposal
+    this.linkedElements = [];
+
+    if ( PHET_IO_ENABLED ) {
+
+      // @public (read-only) {IOType} - assigned in initializePhetioObject - the IO type associated with this instance
+      this.phetioType = null;
 
-    // @public (read-only) {boolean} - assigned in initializePhetioObject - When true, included in the PhET-iO state
-    this.phetioState = null;
+      // @public (read-only) {boolean} - assigned in initializePhetioObject - When true, included in the PhET-iO state
+      this.phetioState = null;
 
-    // @public (read-only) {boolean} - assigned in initializePhetioObject - When true, values can be get but not set
-    this.phetioReadOnly = null;
+      // @public (read-only) {boolean} - assigned in initializePhetioObject - When true, values can be get but not set
+      this.phetioReadOnly = null;
 
-    // @public (read-only) {string} - assigned in initializePhetioObject - Notes about an instance, shown in the PhET-iO Studio Wrapper
-    this.phetioDocumentation = null;
+      // @public (read-only) {string} - assigned in initializePhetioObject - Notes about an instance, shown in the PhET-iO Studio Wrapper
+      this.phetioDocumentation = null;
 
-    // @public (read-only) {Object} - assigned in initializePhetioObject - The wrapper instance for PhET-iO interoperation
-    this.phetioWrapper = null;
+      // @public (read-only) {Object} - assigned in initializePhetioObject - The wrapper instance for PhET-iO interoperation
+      this.phetioWrapper = null;
 
-    // @private {boolean} - track whether the object has been initialized.  This is necessary because initialization
-    // can happen in the constructor or in a subsequent call to initializePhetioObject (to support scenery Node)
-    this.phetioObjectInitialized = false;
+      // @private {boolean} - track whether the object has been initialized.  This is necessary because initialization
+      // can happen in the constructor or in a subsequent call to initializePhetioObject (to support scenery Node)
+      this.phetioObjectInitialized = false;
 
-    // @private {number|null} - tracks the indices of started messages so that dataStream can check that ends match starts
-    this.phetioMessageStack = [];
+      // @private {number|null} - tracks the indices of started messages so that dataStream can check that ends match starts
+      this.phetioMessageStack = [];
 
-    // @public (read-only) {boolean} - has the instance been disposed?
-    this.isDisposed = false;
+      // @public (read-only) {boolean} - has the instance been disposed?
+      this.isDisposed = false;
 
-    // @private {string} - 'model' | 'user'
-    this.phetioEventType = null;
+      // @private {string} - 'model' | 'user'
+      this.phetioEventType = null;
 
-    // @private {boolean} - If marked as phetioHighFrequency: true, the event will be omitted when the query parameter phetioEmitHighFrequencyEvents=false, also see option in Client.launchSim()
-    this.phetioHighFrequency = null;
+      // @private {boolean} - If marked as phetioHighFrequency: true, the event will be omitted when the query parameter phetioEmitHighFrequencyEvents=false, also see option in Client.launchSim()
+      this.phetioHighFrequency = null;
 
-    // @private {boolean} - This indicates a (usually high-frequency) event that is required for
-    // visual playbacks, but can be otherwise overwhelming.  For instance, stepSimulationEmitter emits dt's that are critical to playbacks
-    // but not helpful when reading console: colorized.
-    this.phetioPlayback = null;
+      // @private {boolean} - This indicates a (usually high-frequency) event that is required for
+      // visual playbacks, but can be otherwise overwhelming.  For instance, stepSimulationEmitter emits dt's that are critical to playbacks
+      // but not helpful when reading console: colorized.
+      this.phetioPlayback = null;
 
-    // @private {boolean} By default, Studio creates controls for many types of instances.  This option can be set to
-    // false to direct Studio to omit the control for the instance.
-    this.phetioStudioControl = null;
+      // @private {boolean} By default, Studio creates controls for many types of instances.  This option can be set to
+      // false to direct Studio to omit the control for the instance.
+      this.phetioStudioControl = null;
 
-    // @private {boolean} - See docs above
-    this.phetioFeatured = false;
+      // @private {boolean} - See docs above
+      this.phetioFeatured = false;
 
-    // @private {Object|null}
-    this.phetioEventMetadata = null;
+      // @private {Object|null}
+      this.phetioEventMetadata = null;
 
-    // @public {Object} options to pass through to direct child subcomponents, see NodeIO
-    this.phetioComponentOptions = null;
+      // @public {Object} options to pass through to direct child subcomponents, see NodeIO
+      this.phetioComponentOptions = null;
 
-    // @private {LinkedElement[]} - keep track of LinkedElements for disposal
-    this.linkedElements = [];
+    }
 
     if ( options ) {
       this.initializePhetioObject( {}, options );
@@ -145,6 +149,13 @@
      * @protected
      */
     initializePhetioObject: function( baseOptions, options ) {
+
+      this.phetioObjectInitialized = true;
+      this.tandem = options.tandem;
+
+      if ( !PHET_IO_ENABLED ) {
+        return;
+      }
       assert && assert( options, 'initializePhetioObject must be called with options' );
 
       // TODO: garbage-free implementation
@@ -193,7 +204,6 @@
       );
 
       // Unpack options to instance properties
-      this.tandem = options.tandem;
       this.phetioType = options.phetioType;
       this.phetioState = options.phetioState;
       this.phetioReadOnly = options.phetioReadOnly;
@@ -227,8 +237,6 @@
 
       // Register with the tandem registry
       this.tandem.addInstance( this );
-
-      this.phetioObjectInitialized = true;
     },
 
     /**
@@ -331,6 +339,17 @@
      * @public
      */
     dispose: function() {
+      this.isDisposed = true;
+
+      // Dispose LinkedElements
+      this.linkedElements.forEach( function( linkedElement ) {
+        linkedElement.dispose();
+      } );
+      this.linkedElements.length = 0;
+
+      if ( !PHET_IO_ENABLED ) {
+        return;
+      }
       var self = this;
       assert && assert( !this.isDisposed, 'PhetioObject can only be disposed once' );
 
@@ -350,14 +369,6 @@
         this.tandem.removeInstance( this );
         this.phetioWrapper && this.phetioWrapper.dispose && this.phetioWrapper.dispose();
       }
-
-      // Dispose LinkedElements
-      this.linkedElements.forEach( function( linkedElement ) {
-        linkedElement.dispose();
-      } );
-      this.linkedElements.length = 0;
-
-      this.isDisposed = true;
     }
   }, {
     DEFAULT_OPTIONS: DEFAULTS // the default options for the phet-io object
Index: axon/js/Emitter.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- axon/js/Emitter.js	(revision 73e761ec58a4890a17a5b7fd3d02be0ed32e217c)
+++ axon/js/Emitter.js	(date 1544591611000)
@@ -56,14 +56,16 @@
       this.numberOfArgs = options.argumentTypes.length;
 
       //@private
-      this.assertEmittingValidValues = assert && function() {
-        var args = arguments;
-        assert( args.length === this.numberOfArgs,
-          `Emitted unexpected number of args. Expected: ${this.numberOfArgs} and received ${args.length}` );
-        for ( let i = 0; i < options.argumentTypes.length; i++ ) {
-          assert( TypeDef.validValue( args[ i ], options.argumentTypes[ i ] ), 'value is unexpected type: ' + args[ i ] );
-        }
-      };
+      if ( assert ) {
+        this.assertEmittingValidValues = function() {
+          var args = arguments;
+          assert( args.length === this.numberOfArgs,
+            `Emitted unexpected number of args. Expected: ${this.numberOfArgs} and received ${args.length}` );
+          for ( let i = 0; i < options.argumentTypes.length; i++ ) {
+            assert( TypeDef.validValue( args[ i ], options.argumentTypes[ i ] ), 'value is unexpected type: ' + args[ i ] );
+          }
+        };
+      }
 
       // @private {function[]} - the listeners that will be called on emit
       this.listeners = [];

[samreid]

Some thoughts:

1. I don't want to send this sim to dev testing if we think we are going to redo a lot of the implementation focusing around memory savings.
2. On the other hand, requesting QA dev testing focused on iPad 2 memory and crashing may be good to help us determine whether the sim is acceptable or better understand the nature of the crashing.
3. Let's say we determine that the sim is crashing too frequently on iPad 2 and we cannot publish. Then we need to do one of the following:

a) reduce general memory usage without changing functionality--maybe we can find parts of common code or the sim heap that are too bulky and can be reduced. However, it would be unfortunate if this pushes us toward implementing things in less maintainable ways.
b) reduce feature set. It would be unfortunate if we had to start dropping features in order to stay below a memory threshold. For instance, commenting out the WaveMeterNode saves 5MB total. Another option we could pursue is to somehow figure out a way to publish each screen as its own sim and use those on iPad 2 (I have no idea how we could do that in practice and it would be an inferior user experience).

I fuzz tested Graphing Quadratics 1.0 on this same iPad2 to test for crashing profile, as a comparison.
First crash at 15 seconds
Second test, still no crashes fuzzing after 15 minutes
For reference, Graphing Quadratics weighs about 40MB and Wave Interference weighs about 60MB.

One last point--if we add a fourth screen to Wave Interference, the memory usage is just going to increase. For instance, right now each screen weighs around 20MB. If we can get this to around 15MB per sim, we would have around the same memory footprint as graphing quadratics. Then if we add another screen, this will just push us back where we started.

Another possibility--that the crash is due more to memory thrashing (allocation and deallocation) than it is to static size. If that proves to be the case, we could work toward reducing allocations during the runtime of the sim, but this would generally entail changing toward less maintainable code.

UPDATE: Another data point: testing the built Wave Interference dev.51 on iPad 2 shows the same behavior as before. First crash at around 10 seconds, then next run goes for >10 minutes without crashing.

@ariel-phet what do you think and how do you recommend to proceed?

[samreid]

Another possibility: even though the allocated canvas sizes don't show up on the heap, they may still contribute to iPad 2 crashing. It would lead to very hackish code but we could try creating only one LatticeCanvasNode and passing it to each screen. I'm not sure how layering would work or if scenery still allocates multiple <canvas> elements in that case. It could take a few hours to test this out and may or may not work.

Here's the URL I've been testing with:
https://phet-dev.colorado.edu/html/wave-interference/1.0.0-dev.51/phet/wave-interference_en_phet.html

[samreid]

Notes from discussion with @ariel-phet:

• dev.51 ran on Ariel's iPad 2 for >5 minutes under normal usage with no crashing.
• We will be dropping support for iPad 2 soon, probably before Wave Interference 2.0 is released
• Equality Explorer weighs in at 70MB
• We will give to the QA team and ask to keep an eye out for crashing.
• If the sim happens to crash sometimes (but not all the time) on iPad 2, this wouldn't prevent publication of 1.0.

[samreid]

@KatieWoe were there any iPad 2 tests run during the dev test? Here is a note from the dev request:

In addition to the general QA Dev testing, please test crashing conditions on iPad 2. How long can you use the sim before it crashes? Does any particular conditions lead to crashing? Please test multiple trials and report results at #128. Per @ariel-phet's direction, take a little bit of time exploring this, but not too long.

[KatieWoe]

I did some testing for about 7 - 10 minutes and did not see any crashes. @samreid

[samreid]

Sounds good, thanks for testing, closing.