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

Performance improvements for ChromeOS #198

Closed
jessegreenberg opened this issue Feb 21, 2020 · 17 comments
Closed

Performance improvements for ChromeOS #198

jessegreenberg opened this issue Feb 21, 2020 · 17 comments

Comments

@jessegreenberg
Copy link
Contributor

jessegreenberg commented Feb 21, 2020

Frome phetsims/qa#470, Performance of the Graphs screen on ChromeOS was reported as "fair" and it was requested that we look into performance improvements for that screen.

@jessegreenberg
Copy link
Contributor Author

I suspect that performance hit is coming from the graphs and griddle as a lot of animation. To verify it would be helpful to see where time is spent on the device. @KatieWoe and I will meet today to investigate (either with screen share or setting up remote access).

@jessegreenberg
Copy link
Contributor Author

I am not able to see where time is being spent on ChromeOS, im inspecting a build with --minify.mangle=false. @KatieWoe helped me set up remote access but the dev tools don't really show anything.

With the same build on my Windows 10 machine I can see the following however on Chrome:
image

Which shows a substantial amount of time spent drawing the skater plot.

@jessegreenberg
Copy link
Contributor Author

drawSkatterData is slow because we draw every single point with fill/stroke, rather than drawing them all at once. But that is necessary because different points could have different colors and opacity.

@jessegreenberg
Copy link
Contributor Author

In the above commit I removed the repaint when opacity of a data point changes. This is massively inefficient because it repaints the canvas when ever a single data point's opacity changes. Over slack I asked @KatieWoe to see if removing this improves things and she said that performance is

Noticeably better. Still not fantastic, but better.

This code can be fully removed because of changes to point behavior in #148, we can get away with redrawing the plot only when new data is added/removed.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Feb 21, 2020

The data point removal could probably be improved. Every time a point is removed the plot is redrawn. And the behavior currently is that we remove many points at once. So there are frames where we remove ~200 points at once, and therefore redraw the canvas ~200 times in a single frame.

@jessegreenberg
Copy link
Contributor Author

The above commits support #198 (comment).

@jessegreenberg
Copy link
Contributor Author

@KatieWoe can you please check performance of the graphs screen in this version?
https://phet-dev.colorado.edu/html/energy-skate-park/1.0.0-dev.8/phet/energy-skate-park_en_phet.html

@KatieWoe
Copy link
Contributor

Seems to run faster but definitely has jumpy behavior where it slows down a fair amount.

@jessegreenberg
Copy link
Contributor Author

Upon further inspection, I found that the canvas for the graph is repainted with updateDisplay and invalidatePaint() signifies s that it must be repainted on the next updateDisplay. And so I don't expect any performance improvement from #198 (comment). In fact, I am going to revert the above two commits because I don't expect them to benefit us.

@jessegreenberg
Copy link
Contributor Author

On RequireJS I caught this time spent
image

Can see much time spent in griddle, that is likely still the bottleneck.

For the same time I see some time in the model
image

But half of that is validation (captured for requireJS) and I am not convinced that is taking enough time to investigate improvements.

@jessegreenberg
Copy link
Contributor Author

So another way to improve performance could be to reduce the fadeaway effect. Comparing two builds, master vs a build with much faster fade out (with minify.mangle=false) :
Build off of master:
image

Fast fade:
image

We see about 8% less time across similar time spans with shorter fade out. I think opacity is hard on peformance, but this also reduces the number of points to be drawn in a given time.

@jessegreenberg
Copy link
Contributor Author

jessegreenberg commented Feb 27, 2020

Done in the above commit. On Windows Chrome with ?profiler, I could see a noticeable dip to ~40-50 fps when fading out many points. After above commit this is gone. Actually, I think this looks nicer as well.

@jessegreenberg
Copy link
Contributor Author

@KatieWoe could you please do another performance check on ChromeOS for this version?
https://phet-dev.colorado.edu/html/energy-skate-park/1.0.0-dev.9/phet/energy-skate-park_en_phet.html

@jessegreenberg jessegreenberg removed their assignment Feb 27, 2020
@KatieWoe
Copy link
Contributor

Better. Still there, but less obvious. I'm using a different device right now, so I may need to look at the other one later to confirm for sure.

@KatieWoe
Copy link
Contributor

KatieWoe commented Feb 28, 2020

On the original chromebook it looks about the same (as the last dev version).

@KatieWoe KatieWoe assigned jessegreenberg and unassigned KatieWoe Feb 28, 2020
@jessegreenberg
Copy link
Contributor Author

This issue was all about performance on the Graphs screen. The graphs on that screen have been entirely re-implemented to use griddle and so I am curious if hat has had an impact on this issue. Removing assignment for now, but will flag in a future QA test.

@jessegreenberg jessegreenberg removed their assignment Jul 1, 2020
@jessegreenberg
Copy link
Contributor Author

The sim has passed QA testing and performance checks in the most recent RC and publication. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants