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

iPad2 Crash #62

Closed
samreid opened this issue Jul 3, 2017 · 82 comments
Closed

iPad2 Crash #62

samreid opened this issue Jul 3, 2017 · 82 comments
Assignees
Labels

Comments

@samreid
Copy link
Member

samreid commented Jul 3, 2017

This happened every couple of minutes on the iPad2. Discovered during testing in #47

@samreid samreid self-assigned this Jul 3, 2017
@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

I saw a few circumstances on iPad2 where objects would no longer drag (though some items were tappable), perhaps related to this issue?

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

Previously noted in phetsims/circuit-construction-kit-common#370

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

Running with ?fuzzMouse on desktop, I'm still seeing

WebGLBlock.js?bust=1499092334857:80 WARNING: Too many active WebGL contexts. Oldest context will be lost.

And running ?fuzzMouse on the iPad seems like a reliable way to reproduce this problem.

So far popupDisplay has been unresponsive during fuzzMouse.

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

Joist disables popping up windows while fuzzing:

    // override window.open with a semi-API-compatible function, so fuzzing doesn't open new windows.
    if ( phet.chipper.queryParameters.fuzzMouse ) {
      window.open = function() {
        return {
          focus: function() {},
          blur: function() {}
        };
      };
    }

I had to comment that out to get this working. When I did, I saw that ValueNodes are sneaking in between the webgl nodes and some changes:

image

samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Jul 3, 2017
@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

I fixed the layering above and now it seems the WebGL layer isn't getting split up. Still need to test on iPad2.

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

I published the fix in http://www.colorado.edu/physics/phet/dev/html/circuit-construction-kit-dc/1.0.0-dev.77/circuit-construction-kit-dc_en.html but it still crashed after 20 seconds of fuzzing on the iPad2.

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

When running tethered with Safari, the app crash closes the tether page, so it appears I cannot use that for diagnostic.

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

Using xcode + wkWebView I was able to load a profilable version of the app. It shows we are using 124MB on startup:
image

Dragging out a bunch of wires gets up to 167MB, then it crashed
image

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

Running with ?webgl=false starts at 94Mb. Dragging everything out of the Lab carousel page 1 leads to 160MB (still not crashed).

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

I used the WKWebView example from http://applecocoa.com/swift-3-wkwebview/

Had to restart iPad when instrument was loading blank.

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

With ?screens=1 I'm still seeing 100MB usage, then up to 140MB with everything dragged out of the toolbox.

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

Even when I added this cache busting code, it was still loading a cached version:
https://stackoverflow.com/questions/27105094/how-to-remove-cache-in-wkwebview

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

I disabled all caching in my apache and that worked:
https://stackoverflow.com/questions/11532636/how-to-prevent-http-file-caching-in-apache-httpd-mamp

But using image! instead of mipmap! I'm still seeing 122MB used.

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

Summarizing findings:
plain: 124MB -> 167MB + crash
image plugin: 122MB
screens=1: 100MB -> 140MB
webgl=false: 94MB -> 160MB

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

@jonathanolson and I hit "edit" in the safari dev tools (basically following https://webkit.org/blog/6425/memory-debugging-with-web-inspector/ ) and saw this breakdown:

image

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

After building with accessibility:false:

image

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

The above readings may be compromised, we may need to restart safari between recordings.

Here's a fresh safari with tiny.png being loaded for every image.
image

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

Here's a fresh safari with all defaults (including webgl)

image

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

@jonathanolson suggested trying these changes to Node.toDataURLNodeSyncrhonous:

    toDataURLNodeSynchronous: function( x, y, width, height ) {
      assert && assert( x === undefined || typeof x === 'number', 'If provided, x should be a number' );
      assert && assert( y === undefined || typeof y === 'number', 'If provided, y should be a number' );
      assert && assert( width === undefined || ( typeof width === 'number' && width >= 0 && ( width % 1 === 0 ) ),
        'If provided, width should be a non-negative integer' );
      assert && assert( height === undefined || ( typeof height === 'number' && height >= 0 && ( height % 1 === 0 ) ),
        'If provided, height should be a non-negative integer' );
      var result;
      this.toDataURL( function( dataURL, x, y, width, height ) {
        var img = document.createElement( 'img' );
        img.src = dataURL;
        result = new scenery.Node( {
          children: [
            new scenery.Image( img, { x: -x, y: -y, initialWidth: width, initialHeight: height } )
          ]
        } );
      }, x, y, width, height );
      assert && assert( result, 'toDataURLNodeSynchronous requires that' );
      return result;
    },

Seems promising:
image

@samreid
Copy link
Member Author

samreid commented Jul 3, 2017

On the iPad, this is showing as 130MB on startup.

@jonathanolson
Copy link
Contributor

@ariel-phet, would it be an option to find someone familiar with diagnosing memory issues (or who is familiar enough with browser internals) to consult on this? It seems to be an issue we're running into repeatedly lately, and I'm not familiar enough with the tools to easily identify what is taking up the memory, and how we can fix it.

@jonathanolson
Copy link
Contributor

Gaining very little information per hour of investigation.

@ariel-phet
Copy link

@jonathanolson certainly an option...I just am not quite sure where I would find someone with that expertise. I will see if my friend from pivotal has any suggestions on a consultant.

@samreid
Copy link
Member Author

samreid commented Jul 4, 2017

We used to consult with Sam Breed of Quick Left on Pearl, not sure if he is still around.

@samreid
Copy link
Member Author

samreid commented Jul 4, 2017

I noticed there are 521 Property instances upon startup, and Chrome reported phetioValueType took up 3168B per item (if I'm reading this correctly). This should add up to 1.65MB. Running in a WKWebView, I saw 119MiB or 123MiB instead of the usual 124MiB or 130MiB. This may all be in the noise....

@samreid
Copy link
Member Author

samreid commented Jul 4, 2017

I found the safari js allocation tooling

image

@samreid
Copy link
Member Author

samreid commented Jul 7, 2017

Could also do for VertexNode and SolderNode what we did for ChargeNode:

1082 VertexNode 1344 pickable hits inputListeners translated [Trail 1.5.1.92] undefined 230f
1096 Image 65 hits translated [Trail 1.5.1.92.0] undefined 630f (f) self:ImageWebGLDrawable#612
1083 SolderNode 1345 invis I-invis I-rel-invis I-self-invis nofit-ancestor nofit-self unpickable translated [Trail 1.5.1.93] undefined 630f
1097 Image 7 I-invis I-rel-invis I-self-invis nofit-ancestor translated [Trail 1.5.1.93.0] undefined 630f (f) self:ImageWebGLDrawable#613
1084 VertexNode 1347 pickable hits inputListeners translated [Trail 1.5.1.94] undefined 230f
1098 Image 65 hits translated [Trail 1.5.1.94.0] undefined 630f (f) self:ImageWebGLDrawable#614

@samreid
Copy link
Member Author

samreid commented Jul 7, 2017

Perhaps a good time to test this hypothesis?

Perhaps many allocations triggers a GC which spikes the memory and crashes it?

@samreid
Copy link
Member Author

samreid commented Jul 7, 2017

Investigation in reducing allocations while dragging seems promising (though with the caveat that it seems there are unknown variables here and it is perhaps difficult to reproduce experiments exactly without rebooting the iPad between each run).

UPDATE: but the last result in #74 was without a reboot

@samreid
Copy link
Member Author

samreid commented Jul 18, 2017

3 minutes and 47 seconds of continuous use on iPad2 and no crash. However I did find a bug that the cut button isn't cutting wires.

@samreid
Copy link
Member Author

samreid commented Jul 18, 2017

?fuzzMouse survived iPad2 for 52 seconds.
2nd run: 56 seconds
3rd run: 45 seconds

@samreid
Copy link
Member Author

samreid commented Jul 18, 2017

Looks like fuzzMouse in desktop Chrome is leaking around 5MB every 30 seconds (or ended up allocating 5MB of objects in that time).
image

@samreid
Copy link
Member Author

samreid commented Jul 18, 2017

After running for a while, 90 seconds of fuzzing seems like there is no leak:

image

@samreid
Copy link
Member Author

samreid commented Jul 18, 2017

I think it would be nice to get some QA testing on this when a dev version is ready for QA team testing.

@ariel-phet
Copy link

@samreid once you feel you have a stable version, go ahead and create a QA task, feel free to immediately label it "top-priority" and assign to @phet-steele

@ariel-phet ariel-phet removed their assignment Jul 18, 2017
@samreid
Copy link
Member Author

samreid commented Jul 22, 2017

@phet-steele according to phetsims/qa#26 iPad2 received some dev testing. Any crashes? How thorough was your testing? Can this issue be closed?

@phet-steele
Copy link
Contributor

I had no crashes! The performance was decent but not great. I did not really push it to its limits, nor do I think I really ever had too many objects out at once, though. I was planning on really going at the iPad 2 after the AAPT stuff had blown over, so please keep open.

@samreid samreid removed their assignment Jul 23, 2017
@samreid
Copy link
Member Author

samreid commented Aug 22, 2017

We'll need to test crashiness again after increasing the WebGL backing scale, see #139

@samreid
Copy link
Member Author

samreid commented Sep 27, 2017

Now that we are rendering everything with SVG we should check again for iPad crashiness.

@BryceAG
Copy link

BryceAG commented Sep 28, 2017

While testing some pretty complex circuits it did not crash once.
The performance for electrons started fine but gradually got slower as the complexity of the circuit decreased. Not to unbearable levels, but the frame rate dipped and electrons moved maybe a third of the speed that the conventional arrows did for the same complex circuit. Conventional current kept a good frame rate and stayed moving the same speed regardless of complexity. Both current modes had better performance is schematic mode.
Example complex circuit
cf5c1be0-267a-4256-9ef0-aa02fc7048b8

@BryceAG
Copy link

BryceAG commented Sep 29, 2017

My mistake, what I originally tested on was an iPad air 2. After testing on an iPad 2 it did not crash and the same trends are true (conventional performs better than electrons, and schematic better than realistic), but the performance was much worse across all modes, and decreasing much faster. It was showing 3 fps in a circuit similar to above.

@samreid
Copy link
Member Author

samreid commented Sep 29, 2017

Thanks for your testing Bryce. Since it no longer crashes on iPad2, will you please close this issue and create a new issue about dropping to 3 frames per second in that case? Assign the new issue to @ariel-phet to decide if we want to address it.

@BryceAG
Copy link

BryceAG commented Sep 29, 2017

moved to #160

@BryceAG BryceAG closed this as completed Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants