710563 - Refresh driver should use REPEATING_PRECISE_CAN_SKIP timer by default

Status: REOPENED

(Core :: Web Painting, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

A SLACK timer has the effect of causing animations to drift away from 60fps depending on how long the repaint callback takes.  For example, a callback that takes 16ms will get 30fps, and one that takes 8ms will get 40fps, even though both can drive 60fps animations.  The only way to achieve full 60fps is with a callback that takes 0ms.  (And of course throughout I'm assuming perfect timers.)

We should try as hard as we can to hit 60fps.  The simplest way of doing that in the current code is to switch to REPEATING_PRECISE_CAN_SKIP wholesale, not just for requestanimationframe callbacks.

In b2g's home screen, we saw a nice qualitative perf improvement when flipping the pref to always use CAN_SKIP.  However, the home screen uses RAF with webgl and a simple CSS animation, which are supposed to flip on CAN_SKIP already.  So now I'm not so sure what was happening with b2g.  mwu did you get any quantitative data from the pref flip?

However, I think we should make this change anyway.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Use REPEATING_TIMER_PRECISE_CAN_SKIP instead of SLACK to try to stay closer to 60fps rendering

Comment 2

[Michael Wu [:mwu]]

Well we had the FPS counter on. That doesn't give the best data but it clearly went from hovering around 35 FPS to around 55 FPS.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 581561 [details] [diff] [review]
Use REPEATING_TIMER_PRECISE_CAN_SKIP instead of SLACK to try to stay closer to 60fps rendering

> However, the home screen uses RAF with webgl and a simple CSS animation, which
> are supposed to flip on CAN_SKIP already.

CSS stuff doesn't.  RAF does.  Can you fix the comment accordingly?

r=me.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Speaking of which, can you just write requestAnimationFrame?  RAF is the Royal Air Force.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Comment on attachment 581561 [details] [diff] [review]
> Use REPEATING_TIMER_PRECISE_CAN_SKIP instead of SLACK to try to stay closer
> to 60fps rendering
> 
> > However, the home screen uses RAF with webgl and a simple CSS animation, which
> > are supposed to flip on CAN_SKIP already.
> 
> CSS stuff doesn't.  RAF does.  Can you fix the comment accordingly?
> 

Oh hm.  I asked and was told that it does.  Will fix.

(In reply to David Baron [:dbaron] from comment #4)
> Speaking of which, can you just write requestAnimationFrame?  RAF is the
> Royal Air Force.

OK, but I could also say that FPS means "first person shooter".  If requestAnimationFrame is abbreviated, is "raf" better?  (My fingers are lazy.)

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Looking at

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=30e270816255,221eccfa6a3f,ceffbd680e2c,0c46aac6a23f,96a644cb3158,9fae755f5103,58a278555680&newRev=20521568a931&tests=a11y,tdhtml,tdhtml_nochrome,a11y_paint,tdhtml_paint,tdhtml_nochrome_paint,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,tp5,tp5_memset,tp5_pbytes,tp5_rss,tp5_shutdown,tp5_xres,tp5_paint,tp5_memset_paint,tp5_pbytes_paint,tp5_rss_paint,tp5_shutdown_paint,tp5_xres_paint,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tsspider_paint,tsspider_nochrome_paint,tgfx,tgfx_nochrome,tgfx_paint,tgfx_nochrome_paint,tscroll,tsvg,tsvg_opacity,ts,ts_paint,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen,tpaint&submit=true

we might have a big regression on "tp5_shutdown_paint" on win7 (not sure what that is ...) and a general regression on "tdhtml_nochrome_paint".  Will kick off another set of runs.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Similar results for tdhtml_paint and tdhtml_nochrome_paint.  I don't have any guesses about what's causing this, but I suspect some timing is being altered a bit.

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=30e270816255,221eccfa6a3f,ceffbd680e2c,0c46aac6a23f,96a644cb3158,9fae755f5103,58a278555680&newRev=20521568a931&tests=a11y,tdhtml,tdhtml_nochrome,a11y_paint,tdhtml_paint,tdhtml_nochrome_paint,tp4,tp4_memset,tp4_pbytes,tp4_rss,tp4_shutdown,tp4_xres,tp5,tp5_memset,tp5_pbytes,tp5_rss,tp5_shutdown,tp5_xres,tp5_paint,tp5_memset_paint,tp5_pbytes_paint,tp5_rss_paint,tp5_shutdown_paint,tp5_xres_paint,dromaeo_basics,dromaeo_css,dromaeo_dom,dromaeo_jslib,dromaeo_sunspider,dromaeo_v8,tsspider,tsspider_nochrome,tsspider_paint,tsspider_nochrome_paint,tgfx,tgfx_nochrome,tgfx_paint,tgfx_nochrome_paint,tscroll,tsvg,tsvg_opacity,ts,ts_paint,ts_cold,ts_cold_generated_max,ts_cold_generated_max_shutdown,ts_cold_generated_med,ts_cold_generated_med_shutdown,ts_cold_generated_min,ts_cold_generated_min_shutdown,ts_cold_shutdown,ts_places_generated_max,ts_places_generated_max_shutdown,ts_places_generated_med,ts_places_generated_med_shutdown,ts_places_generated_min,ts_places_generated_min_shutdown,ts_shutdown,twinopen,tpaint&submit=true

I'm not going to have time to investigate this deeply for a while :(.  Un-assigning myself.  Someone else, please feel free to steal!

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/0dbdc8243364

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Ugh, sorry, I typo'd the first bug number in the comment.  The b2g "preview landing" was bug 717532.  Talos issues here still to be sorted out.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Whups, mid-aired.