phetsims / ohms-law

"Ohm's Law" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/ohms-law
GNU General Public License v3.0
5 stars 6 forks source link

Low Performance on iPad Air 2 #132

Closed KatieWoe closed 5 years ago

KatieWoe commented 5 years ago

For https://github.com/phetsims/QA/issues/289 On iOS 12.1.4 iPad Air 2 the performance dropped significantly when using the sliders. ?profiler showed as low as 13 fps. Averaged around 20, could be as high as 40.

jessegreenberg commented 5 years ago

I only have access to an iPad2, but I can confirm that the performance has degraded on that platform since the published version.

jessegreenberg commented 5 years ago

I tried disabling the alerts, then all a11y, then sound (by muting in the navigation bar). The sim a bit got faster after doing each, but disabling a11y was by far the most effective.

jessegreenberg commented 5 years ago

I tried disabling positioning of PDOM elements, but I didn't see any improvement.

jessegreenberg commented 5 years ago

Ill check on performance in this version: https://phet-dev.colorado.edu/html/ohms-law/1.4.0-dev.25/phet/ohms-law_en_phet.html?a11y to see if it has bad then or has degraded since.

jessegreenberg commented 5 years ago

Difficult to say, performance might be a little better in that version, but not by much.

jessegreenberg commented 5 years ago

Performance for me is equivalent to the performance of the published version of RIAW on my iPad2. @KatieWoe is that true for iOS 12.1.4 iPad Air 2 as well?

KatieWoe commented 5 years ago

Does seem fairly comparable. Maybe even a tad better in Ohm's Law

jessegreenberg commented 5 years ago

Here is a thing from Chrome tools, I am not sure what I am looking at yet image

But I see a lot lf "setTextContent" and "setInnerContent". which probably indicates that is what is taking up time?

jessegreenberg commented 5 years ago

Above is the call tree from 'onPointerMove'.

jessegreenberg commented 5 years ago

I just noticed that performance in the a11y view is far worse than it used to be, not sure if it is related to this.

jessegreenberg commented 5 years ago

I just noticed that performance in the a11y view is far worse than it used to be, not sure if it is related to this.

It isn't related to this, see https://github.com/phetsims/chipper/issues/740

jessegreenberg commented 5 years ago

When returning early from AccessibilityUtil.setTextContent, the performance improves a bit. This in combination with disabling sound gets the performance closer to the published version.

Here is the call tree now: image

jessegreenberg commented 5 years ago

Within that call tree I am seeing setAttributeToElement and setInputValue taking together ~30% of the time taken for the event (total 0.5 ms). image

EDIT: Chrome is reporting that these operations take about as much time as updating Text readout from the voltage label image

jessegreenberg commented 5 years ago

Doing profiling with @twant and @zepumph, we tried to replace using innerHTML with textContent in AccessibilityUtil.setTextContent and discovered that the innerHTML is taking up a large amount of time relative to textContent. image

image

These images show that innerHTML took up ~25% of the computation time during the test, while using textContent took up about 5%.

jessegreenberg commented 5 years ago

We tried batching changes to PDOM elements in https://github.com/phetsims/scenery/issues/663, but discovered that it isn't giving us much of a performance benefit because inner content is actually not updating much more frequently than once per element per animation frame. So we should reconsider how to optimize.

jessegreenberg commented 5 years ago

I found this jsperf for innerHTML vs textContent vs innerText and I see that innerHTML is 99% slower, which matches our testing results above. image

jessegreenberg commented 5 years ago

I opened https://github.com/phetsims/scenery/issues/955 to see if we can use textContent more than innerHTML because it is faster. But the dynamic descriptions in this sim use innerHTML so we wont see a performance improvement from that issue unless we can get rid of the formatting tags in this sim.

jessegreenberg commented 5 years ago

Here is another test that shows setting innerHTML is slower than setting innerText of a strong tag: http://jsben.ch/yH5vq

The block test sets textContent of a strong tag under a p, and the second test sets innerHTML of a p to be <strong>Test!</strong>

jessegreenberg commented 5 years ago

With this test, I proceeded with https://github.com/phetsims/scenery/issues/955 and removed all formatting tags from a11y strings. Before change, setTextContent takes 15% of running time. 15

After change, setTextContent takes 5% of running time: 5

jessegreenberg commented 5 years ago

A built version of OL with the changes in https://github.com/phetsims/ohms-law/issues/132#issuecomment-475008440 runs a little faster on an iPad2, but still slower than the published sim.

jessegreenberg commented 5 years ago

In a number of screenshots above I see that layout and Update Layer Tree and recalculate style are all tied to AccessibilityUtil. When I click on AccessibilityUtil links in the dev tools, it takes me to the setting of textContent. I think that is the next place to investigate.

jessegreenberg commented 5 years ago

In https://github.com/phetsims/scenery/issues/663 we found that clip will improve the performance of the PDOM while keeping the DOM interactive with AT. Performance improvement was found on my iPad2. Over to https://github.com/phetsims/QA/issues/303 to see if this benefit carries over to the iPad air.

KatieWoe commented 5 years ago

Will need to wait a bit for the iPad Air 2's to come back, but I did a check on the iPad 2 with QA and can confirm that it looks much better now on that platform at least.

KatieWoe commented 5 years ago

On an iOS 11 iPad Air 2 I saw that the average fps of the new dev version was about 5 to 10 fps higher than the average fps of the old dev version.

jessegreenberg commented 5 years ago

Thanks @KatieWoe, that is an improvement. However, I also recently got an iPad Air 2 and was able to see that the performance is noticeably slower compared to the published version. But I also noticed that performance on iPad air 2 is substantially faster when the resistance is low. This indicates that a11y is no longer the performance bottleneck because a11y changes are not dependent on amount of resistance.

We should look into if anything about the dot visual rendering changed.

EDIT: Unsure whether the slowdown caused by number of dots in the wire or size of letters. EDIT2: Probably the dots. The V and I can grow quite large and performance remains good.

jessegreenberg commented 5 years ago

I verified that the sim has the same poor performance with high resistance when both sound and accessibility are disabled.

jessegreenberg commented 5 years ago

When I remove the clipArea on for the dots performance improves substantially.

EDIT: When I change the clipArea to

    dotsNode.clipArea = Shape.rect( -this.width / 2, -this.height / 2, this.width, this.height );

performance is still poor. I thought using ellipticalArc might have been the cause of the slowdown but that is not the case.

jessegreenberg commented 5 years ago

Changing the renderer to canvas helps performance mostly, but there is some slowdown when dragging the resistance thumb in areas of high resistance.

jessegreenberg commented 5 years ago

I just noticed that clipArea is a recent addition since the last publication of this simulation so that is why the published sim is so much faster. I am going to change the implementation so that dots are contained within the wire shape without using clip shape.

jessegreenberg commented 5 years ago

This is working great and performance is much faster on iPad Air 2. I am going to go ahead and close since I have access to an iPad air 2 now.

jessegreenberg commented 5 years ago

To summarize what we looked into and changed to improve performance in this issue:

We discovered that DOM operations were taking up too much time in Chrome and Safari and so we we started https://github.com/phetsims/scenery/issues/663 to reduce the number of DOM changes that occur each animation frame. This included changes to innerHTML and other HTML attributes that were identified as slow in Chrome and Safari. We discovered that this was not as relevant to ohms-law because this sim doesn't have a large number of changes per frame. That work was in scenery and was not merged with master, but was kept on a branch.

We discovered that changing the PDOM styling so that elements are hidden with clip greatly improved performance on iPad 2 (https://github.com/phetsims/scenery/issues/663). But didn't have an impact on performance on an iPad Air 2. This change was still committed to master.

The final change was not related to a11y but to the clipArea used in this sim. By removing usage of clipArea with a large number of dots in the ResistorNode and hiding extra dots in other ways, the performance improved dramatically.