phetsims / ph-scale

"pH Scale" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/ph-scale
GNU General Public License v3.0
0 stars 7 forks source link

performance of "H3O+/OH- Ratio" has degraded #83

Closed ghost closed 5 years ago

ghost commented 5 years ago

Test Device

Hopper, Leibniz

Operating System

iOS 12.3.1

Browser

Mobile Safari 12

Problem Description

For https://github.com/phetsims/QA/issues/323. In the "My Solutions" screen, if you change the pH of the solution with the hydronium / hydroxide ratio checkbox checked, the sim lags quite a bit.

Visuals

This video was taken on Hopper. Hopper is our best iOS 12 device in terms of performance.

https://drive.google.com/open?id=1Us3t7FZacj1l7jADrw0Ppb9-fFq8aKMr

pixelzoom commented 5 years ago

Thanks for reporting.

Drawing hundreds of molecules doesn't come for free, so it's expected that the will be some difference in responsiveness between having H3O+/OH- Ratio "on" vs "off". And the implementation (and performance) in this RC is identical to the published version. That said...

I tested on both ends of the iPad spectrum (iPad6 + iOS12, iPad2 + iOS9) and performance seems more than acceptable. @arouinfar please confirm. If it's not acceptable, I'll investigate whether there's a more efficient way to draw the molecules.

arouinfar commented 5 years ago

And the implementation (and performance) in this RC is identical to the published version.

I disagree @pixelzoom. I compared the latest published version (1.2.18) to the rc (1.3.0-rc.1) on both Hopper (iPad 6, iOS 12) and Leibniz (iPad Air 2, iOS 12). Latest has staggeringly better performance, and I did not observe any lag at all.

Similar to #82, I think it would be worthwhile to investigate why the performance has tanked in the 1.3 branch.

pixelzoom commented 5 years ago

@arouinfar said:

I think it would be worthwhile to investigate why the performance has tanked in the 1.3 branch.

Here are the results of investigation.

Rendering of H3O+/OH- Ratio is performed in RatioNode.

This issue and https://github.com/phetsims/ph-scale/issues/82#issuecomment-498413510 are identical, because RatioNode is not updated until it becomes visible. So all of these are equivalent, causing RatioNode to recompute and redraw:

In https://github.com/phetsims/ph-scale/issues/82#issuecomment-498413510, @arouinfar noted that:

... Interestingly, latest (1.2.18) is buttery smooth ...

Looking through changes since the 1.2 release branch was created on 11/17/2015... The only significant change is https://github.com/phetsims/ph-scale/commit/8b1ee98ac174a74521937741bae80ccaa5bf8068, where toImage was replaced by toCanvas for phetsims/scenery#250. But that occurs once, on instantiation, and should not affect the performance of updates.

Changing from Math.round to DOT/Utils.roundSymmetric in https://github.com/phetsims/ph-scale/commit/932f2afa335833c815259cbaabe1b55970639575 for https://github.com/phetsims/chipper/issues/736 is also a change that has occurred since 1.2. But it's unlikely to have caused degraded performance.

Here's the implementation of roundSymmetric:

    roundSymmetric: function( value ) {
      return ( ( value < 0 ) ? -1 : 1 ) * Math.round( Math.abs( value ) );
    },
pixelzoom commented 5 years ago

The commits above are mostly internal cleanup in RatioNode -- bring doc up to standards, and rename some internal API functions and parameters. https://github.com/phetsims/ph-scale/commit/9d15a5d985e72a71df236feffa0e26bc1ab35986 is the only significant change, a simplification of createRandomX and createRandomY.

pixelzoom commented 5 years ago

I've been through RatioNode with a fine-toothed comb, and I can't find anything that would explain this issue. RatioNode is using what has proven to be the most efficient approach for drawing large numbers of similar objects (Canvas drawImage). And the same approach is used in Gas Properties, Acid-Base Solutions, Plinko Probability, ...

I also cannot reproduce this on my test machines.

@arouinfar said:

... I compared the latest published version (1.2.18) to the rc (1.3.0-rc.1) on both Hopper (iPad 6, iOS 12) and Leibniz (iPad Air 2, iOS 12). Latest has staggeringly better performance, and I did not observe any lag at all.

I did the same comparison with my iPad2 + iOS 9.3.5 and my iPad6 + iOS 12.3.1. I can see no difference in responsiveness between 1.2.18 and 1.3.0-rc.1. So I can't reproduce, and I can't explain why the performance would be radically different on two iPad6 models (mine and Hopper) both running iOS 12.3.1.

Questions for @arouinfar:

(1) What version of iOS is Hopper running? Is it the latest, iOS 12.3.1? Your comment and the PHET Asset Inventory indicate "iOS 12". (Inventory correction requested in https://github.com/phetsims/QA/issues/347.)

(2) What version of iOS is Leibniz running? Your comment says "iOS12", but the PHET Asset Inventory says "iOS 9.3.5". (Inventory correction requested in https://github.com/phetsims/QA/issues/347.)

(3) This issue concerns me for Gas Properties, because (as mentioned above) pH Scale and Gas Properties use an identical approach for drawing particles. Can you please test Gas Properties on these same iPads (Hopper and Leibniz) and report on performance?

arouinfar commented 5 years ago

(1) What version of iOS is Hopper running? Is it the latest, iOS 12.3.1? Your comment and the PHET Asset Inventory indicate "iOS 12". (Inventory correction requested in phetsims/QA#347.)

Version 12.3.1

(2) What version of iOS is Leibniz running? Your comment says "iOS12", but the PHET Asset Inventory says "iOS 9.3.5". (Inventory correction requested in phetsims/QA#347.)

It's also on version 12.3.1. The QA Simplified Inventory tab (which maps devices to locations in the device cart) had it listed as iOS 12. I've updated Hopper and Leibniz in the spreadsheet.

(3) This issue concerns me for Gas Properties, because (as mentioned above) pH Scale and Gas Properties use an identical approach for drawing particles. Can you please test Gas Properties on these same iPads (Hopper and Leibniz) and report on performance?

In https://github.com/phetsims/gas-properties/issues/98#issuecomment-500497481 I tested Gas Props with Leibniz and Pauling, and there were no performance issues. Testing dev.38 on Hopper showed similar performance in the worst case scenario on the Energy screen (everything on/open, all 2000 particles in the box with pressure near max).

@pixelzoom I recorded screen captures of Hopper, Leibniz, and Pauling to show the degraded performance in the 1.3 branch.

  1. Open My Solution
  2. Set pH to max or min (either 15.0 or -1.0)
  3. Turn on H3O+/OH- Ratio checkbox
  4. Rapidly drag H3O+ or OH- readout on the Concentration scale to change pH between extremes of 15.0 and -1.0.

Hopper, iPad 6 running iOS 12.3.1

Video shows 1.2.18 (latest) and then 1.3.0-rc.1. There is significant lag in changing the pH with H3O+/OH- Ratio on in 1.3.0-rc.1.

https://drive.google.com/open?id=1plD6IsDb3X5qAK3D2PBrczSm9rAV2H2B

Troubleshooting Info: Hopper - 1.2.18 Name: ‪pH Scale‬
URL: https://phet.colorado.edu/sims/html/ph-scale/latest/ph-scale_en.html
Version: 1.2.18 2019-04-28 12:50:54 UTC
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (iPad; CPU OS 12_3_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.1 Mobile/15E148 Safari/604.1
Language: en-US
Window: 980x735
Pixel Ratio: 2/1
WebGL: WebGL 1.0
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.00)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 8 uniform: 128
Texture: size: 4096 imageUnits: 8 (vertex: 8, combined: 8)
Max viewport: 4096x4096
OES_texture_float: true
Dependencies JSON: {"assert":{"sha":"3921bc37","branch":"HEAD"},"axon":{"sha":"5cacf7c5","branch":"HEAD"},"babel":{"sha":"68fda05f","branch":"master"},"brand":{"sha":"7b96db1a","branch":"HEAD"},"chipper":{"sha":"b492a250","branch":"HEAD"},"dot":{"sha":"660f4241","branch":"HEAD"},"joist":{"sha":"cf144892","branch":"HEAD"},"kite":{"sha":"5ba9a6bb","branch":"HEAD"},"nitroglycerin":{"sha":"fda44e12","branch":"HEAD"},"ph-scale":{"sha":"8368ae64","branch":"HEAD"},"phet-core":{"sha":"35278ca5","branch":"HEAD"},"phetcommon":{"sha":"3a6f5f59","branch":"HEAD"},"phetmarks":{"sha":"3b469560","branch":"HEAD"},"scenery":{"sha":"7f81c8d4","branch":"HEAD"},"scenery-phet":{"sha":"647eac9d","branch":"HEAD"},"sherpa":{"sha":"f3cf6285","branch":"HEAD"},"sun":{"sha":"158de3d2","branch":"HEAD"},"tandem":{"sha":"e5842f89","branch":"HEAD"}}
Troubleshooting Info: Hopper - 1.3.0-rc.1 Name: ‪pH Scale‬
URL: https://phet-dev.colorado.edu/html/ph-scale/1.3.0-rc.1/phet/ph-scale_en_phet.html‬
Version: 1.3.0-rc.1 2019-05-24 13:31:40 UTC‬
Flags: pixelRatioScaling‬
User Agent: Mozilla/5.0 (iPad; CPU OS 12_3_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.1 Mobile/15E148 Safari/604.1‬
Language: en-US‬
Window: 980x735‬
Pixel Ratio: 2/1‬
WebGL: WebGL 1.0‬
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.00)‬
Vendor: WebKit (WebKit WebGL)‬
Vertex: attribs: 16 varying: 8 uniform: 128‬
Texture: size: 4096 imageUnits: 8 (vertex: 8, combined: 8)‬
Max viewport: 4096x4096‬
OES_texture_float: true‬
Dependencies JSON: {}

Leibniz, iPad Air 2 running iOS 12.3.1

Video shows 1.2.18 (latest) and then 1.3.0-rc.1. This is the only device that exhibited the problematic lag when checking H3O+/OH- Ratio. Notice the large period of time between setting the pH to -1.0 (0:11) and turning on the checkbox (0:17). I tapped on the checkbox 3 times. There was so much lag that I thought that my first tap was off, so I tapped a 2nd time which turned off the checkbox, which meant I had to tap it a 3rd time to actually turn it on.

There is also significant lag in changing the pH with H3O+/OH- Ratio on in 1.3.0-rc.1.

https://drive.google.com/open?id=1vcXqujiB36h5qwcjakz2iGP8hONJmJz9

Troubleshooting Info: Leibniz - 1.2.18 Name: ‪pH Scale‬
URL: https://phet.colorado.edu/sims/html/ph-scale/latest/ph-scale_en.html‬
Version: 1.2.18 2019-04-28 12:50:54 UTC‬
Flags: pixelRatioScaling‬
User Agent: Mozilla/5.0 (iPad; CPU OS 12_3_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.1 Mobile/15E148 Safari/604.1‬
Language: en-US‬
Window: 980x637‬
Pixel Ratio: 2/1‬
WebGL: WebGL 1.0‬
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.00)‬
Vendor: WebKit (WebKit WebGL)‬
Vertex: attribs: 16 varying: 8 uniform: 128‬
Texture: size: 4096 imageUnits: 8 (vertex: 8, combined: 8)‬
Max viewport: 4096x4096‬
OES_texture_float: true‬
Dependencies JSON: {"assert":{"sha":"3921bc37","branch":"HEAD"},"axon":{"sha":"5cacf7c5","branch":"HEAD"},"babel":{"sha":"68fda05f","branch":"master"},"brand":{"sha":"7b96db1a","branch":"HEAD"},"chipper":{"sha":"b492a250","branch":"HEAD"},"dot":{"sha":"660f4241","branch":"HEAD"},"joist":{"sha":"cf144892","branch":"HEAD"},"kite":{"sha":"5ba9a6bb","branch":"HEAD"},"nitroglycerin":{"sha":"fda44e12","branch":"HEAD"},"ph-scale":{"sha":"8368ae64","branch":"HEAD"},"phet-core":{"sha":"35278ca5","branch":"HEAD"},"phetcommon":{"sha":"3a6f5f59","branch":"HEAD"},"phetmarks":{"sha":"3b469560","branch":"HEAD"},"scenery":{"sha":"7f81c8d4","branch":"HEAD"},"scenery-phet":{"sha":"647eac9d","branch":"HEAD"},"sherpa":{"sha":"f3cf6285","branch":"HEAD"},"sun":{"sha":"158de3d2","branch":"HEAD"},"tandem":{"sha":"e5842f89","branch":"HEAD"}}
Troubleshooting Info: Leibniz - 1.3.0-rc.1 Name: ‪pH Scale‬
URL: https://phet-dev.colorado.edu/html/ph-scale/1.3.0-rc.1/phet/ph-scale_en_phet.html
Version: 1.3.0-rc.1 2019-05-24 13:31:40 UTC
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (iPad; CPU OS 12_3_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.1.1 Mobile/15E148 Safari/604.1
Language: en-US
Window: 980x637
Pixel Ratio: 2/1
WebGL: WebGL 1.0
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.00)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 8 uniform: 128
Texture: size: 4096 imageUnits: 8 (vertex: 8, combined: 8)
Max viewport: 4096x4096
OES_texture_float: true
Dependencies JSON: {}

Pauling, iPad Air 2 running iOS 11.4.1

Video shows 1.2.18 (latest) and then 1.3.0-rc.1. Compared to 1.2.18, there is quite a bit of lag in 1.3.0-rc.1 when changing the pH with the H3O+/OH- Ratio checkbox on. However, the lag isn't quite as bad as the iOS 12.3.1 devices.

https://drive.google.com/open?id=1Rs7fiSrcPZHyByoSMTMxnRAuVjafpvmZ

Troubleshooting Info: Pauling - 1.2.18 Name: ‪pH Scale‬
URL: https://phet.colorado.edu/sims/html/ph-scale/latest/ph-scale_en.html
Version: 1.2.18 2019-04-28 12:50:54 UTC
Features missing: fullscreen
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (iPad; CPU OS 11_4_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/11.0 Mobile/15E148 Safari/604.1
Language: en-US
Window: 980x637
Pixel Ratio: 2/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Apple A8X GPU - 112.17.1)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.00)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 8 uniform: 128
Texture: size: 4096 imageUnits: 8 (vertex: 8, combined: 8)
Max viewport: 4096x4096
OES_texture_float: true
Dependencies JSON: {"assert":{"sha":"3921bc37","branch":"HEAD"},"axon":{"sha":"5cacf7c5","branch":"HEAD"},"babel":{"sha":"68fda05f","branch":"master"},"brand":{"sha":"7b96db1a","branch":"HEAD"},"chipper":{"sha":"b492a250","branch":"HEAD"},"dot":{"sha":"660f4241","branch":"HEAD"},"joist":{"sha":"cf144892","branch":"HEAD"},"kite":{"sha":"5ba9a6bb","branch":"HEAD"},"nitroglycerin":{"sha":"fda44e12","branch":"HEAD"},"ph-scale":{"sha":"8368ae64","branch":"HEAD"},"phet-core":{"sha":"35278ca5","branch":"HEAD"},"phetcommon":{"sha":"3a6f5f59","branch":"HEAD"},"phetmarks":{"sha":"3b469560","branch":"HEAD"},"scenery":{"sha":"7f81c8d4","branch":"HEAD"},"scenery-phet":{"sha":"647eac9d","branch":"HEAD"},"sherpa":{"sha":"f3cf6285","branch":"HEAD"},"sun":{"sha":"158de3d2","branch":"HEAD"},"tandem":{"sha":"e5842f89","branch":"HEAD"}}
Troubleshooting Info: Pauling - 1.3.0-rc.1 Name: ‪pH Scale‬
URL: https://phet-dev.colorado.edu/html/ph-scale/1.3.0-rc.1/phet/ph-scale_en_phet.html
Version: 1.3.0-rc.1 2019-05-24 13:31:40 UTC
Features missing: fullscreen
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (iPad; CPU OS 11_4_1 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/11.0 Mobile/15E148 Safari/604.1
Language: en-US
Window: 980x637
Pixel Ratio: 2/1
WebGL: WebGL 1.0 (OpenGL ES 2.0 Apple A8X GPU - 112.17.1)
GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.00)
Vendor: WebKit (WebKit WebGL)
Vertex: attribs: 16 varying: 8 uniform: 128
Texture: size: 4096 imageUnits: 8 (vertex: 8, combined: 8)
Max viewport: 4096x4096
OES_texture_float: true
Dependencies JSON: {}
pixelzoom commented 5 years ago

@arouinfar said:

There was so much lag that I thought that my first tap was off, so I tapped a 2nd time which turned off the checkbox, which meant I had to tap it a 3rd time to actually turn it on.

Did the lag occur every time that you turned the checkbox 'on', or only the first time? (I can't tell from the videos because there are no cursors. Do you know about the ?showPointers query parameter to show where the cursor is in videos?)

arouinfar commented 5 years ago

Did the lag occur every time that you turned the checkbox 'on', or only the first time?

Happens every time.

Do you know about the ?showPointers query parameter to show where the cursor is in videos?)

I had absolutely no idea that this existed! This will be a game changer for iPad interviews.

Leibniz is currently unavailable, but I can check again this afternoon and re-record with ?showPointers if you'd like @pixelzoom.

pixelzoom commented 5 years ago

@arouinfar Thanks for the offer to re-record, but not necessary (I believe you :) I'll plan to borrow one of these devices when I resume working on this ( > July 2).

pixelzoom commented 5 years ago

In https://github.com/phetsims/ph-scale/issues/83#issuecomment-502771424, @arouinfar said:

Leibniz ... This is the only device that exhibited the problematic lag when checking H3O+/OH- Ratio.

This is also very confusing to me. As noted in https://github.com/phetsims/ph-scale/issues/83#issuecomment-502401616, checking the checkbox is the same code path as changing the pH or changing the solute (which changes the pH) -- it calls RatioNode.update.

There have been a number of CanvasNode changes since 11/17/2015, maybe something that affects performance on these platforms. Although that doesn't explain why my iPad6 and Hopper (supposedly identical models with identical iOS) have such different performance.

pixelzoom commented 5 years ago

@arouinfar helped me reproduce this on both Hopper and Leibniz. The key is to set a pH value at the extremes of the range (15 or -1), so that we're drawing the maximum number of particles. Knowing that, I will try again to reproduce on my iPad6.

Steps to reproduce:

  1. start sim, go to My Solutions screen
  2. Set pH to 15 or -1
  3. check the "H3O+/OH- Ratio" checkbox, there will be a delay before particles appear
  4. drag the H30+ or OH- indicators on the scale, it will feel sluggish
pixelzoom commented 5 years ago

I followed the steps above on my iPad6 + iOS 12.3.1, and reproduced the problem. When comparing 1.2.18 to 1.3.0-rc.1, there is a noticeable delay when checking the checkbox, and dragging the indicators does feel more sluggish.

@ariel-phet Since the only changes that I've been able to identify are in CanvasNode, @jonathanolson will need to investigate. Please assign and adjust priority. I'm starting with "high" priority since this is the only issue preventing the next RC test cycle.

ariel-phet commented 5 years ago

@jonathanolson has some time to investigate, I recommended starting with about 2'ish hours, and then we can reevaluate how much time to spend.

jonathanolson commented 5 years ago

Investigating. It appears that master's performance is dominated by drawImage combined with the clipping (disabling the clipArea dramatically improves performance). 1.2 branch seems to use the same general approach. I'll look at scenery's changes to see if I can identify this up-front, but I'll likely fall back to bisecting, as it's fairly quick to identify whether it has fast or slow behavior.

jonathanolson commented 5 years ago

So this has been an interesting one! Turns out the offending commit is https://github.com/phetsims/kite/commit/91e2ccf15423927ec6b21136c27f89570294168c (Oct 12, 2017).

It turns out that when the clip area is specified with the final closing segment AND THEN a "closePath" command, it is significantly slower than if that closing segment is omitted.

To see the Canvas commands issued both before and after the commit:

phet.kite.Shape.rectangle( 525, 455, 450, 125 ).writeToContext( new Proxy( {}, {
  get( target, prop, receiver ) {
    return ( ...args ) => {
      console.log( prop, ...args );
    };
  }
} ) );

resulting in:

// before regression, a6896f6f03d5832ab97aa31394952b47cf966a6b
moveTo 525 455
lineTo 975 455
lineTo 975 580
lineTo 525 580
closePath

// after regression, 91e2ccf15423927ec6b21136c27f89570294168c
moveTo 525 455
lineTo 975 455
lineTo 975 580
lineTo 525 580
lineTo 525 455 <--- added, goes back to the start point
closePath

I'll plan to resolve this by adjusting how Canvas shapes are sent. Notably if the last segment is a lineTo AND the shape is closed, we'll omit the lineTo.

jonathanolson commented 5 years ago

I ran a snapshot comparison on an assortment of sims using Canvas, and this does not seem to be affecting rendering. It does have the potential to uncover different browser bugs/implementations.

@pixelzoom, can you confirm that this commit fixes the problem?

pixelzoom commented 5 years ago

This does appear to be fixed, thanks @jonathanolson.

I tested on Hopper and Leibniz (test iPads) in Safari. Here are links to the relevant versions:

1.2.18 - current published version 1.3.0-rc.1 - RC version that has the performance problem 1.4.0-dev.2 - dev version that has the fix

@KatieWoe and @arouinfar please verify. Last one to verify please change the issue label to "fixed-awaiting-deploy".

KatieWoe commented 5 years ago

Much better.

pixelzoom commented 5 years ago

I will need to create and patch "ph-scale-1.3" and "ph-scale-basics-1.3" branches for kite.

pixelzoom commented 5 years ago

I guess @arouinfar didn't have time to review this before AAPT. So I'm going to proceed based on @KatieWoe's evaluation.

arouinfar commented 5 years ago

Sorry @pixelzoom! I was planning to test on the iPad Air 2 we brought with us to the conference, but I haven't gotten the chance yet. I'll happily defer to your and @KatieWoe's assessment of the improved performance.

pixelzoom commented 5 years ago

Manually patched in "ph-scale-1.3" and "ph-scale-basics-1.3" branches of kite.

ghost commented 5 years ago

Performance better on Hopper in https://github.com/phetsims/QA/issues/374.

pixelzoom commented 5 years ago

Verified by QA in https://github.com/phetsims/QA/issues/374, closing.