587887 - Refresh driver timer should be 60Hz and TIMER_REPEATING_PRECISE

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comments on my blog suggest that 60Hz is a better guess for the refresh rate, and being higher than 50 it's slightly more "conservative" if we guessed wrong.

I also think that the timer should be TIMER_REPEATING_PRECISE for a steadier frame rate. Is there a problem with starvation there? I would hope that under load timer events don't get any special priority.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Patch is trivial, if this is the right thing to do

Comment 2

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

If refreshes are expensive, it seems better to leave some processing time to other things; that was why I originally chose TYPE_REPEATING_SLACK rather than TYPE_REPEATING_PRECISE.  That said, I think we should actually experiment with how things behave under heavy load to see what works better.  I think we should probably experiment with the refresh rate at some point as well.

(It might be worth have a boolean and an integer pref to allow people to experiment, though we might want to clamp the integer to be at least 10.)

That said, I'm fine with trying this.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Actually I think having prefs is a fine idea. A pref for the rate actually might be useful to someone. I'll do that.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Add prefs

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That patch also changes the default rate to 60Hz, but doesn't flip the "precise" pref, because that change is somewhat more risky.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Flip "precise" pref on

Comment 7

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

Comment on attachment 466534 [details] [diff] [review]
Add prefs

r=dbaron

Comment 8

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

Comment on attachment 466535 [details] [diff] [review]
Flip "precise" pref on

r=dbaron

Comment 9

[Benjamin Smedberg]

What happens if a particular frame takes more than 1/60 of a second? Does the timer then skip a frame, or do they back up? Is there a test to make sure we don't starve?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

mTimeout always advances by mDelay every time the timer fires, so if firings take longer than mDelay for a while, mTimeout will fall behind "now" arbitrarily far and events will be fired constantly. However, we shouldn't starve during this time because we're just posting events to the main thread and events are still processed in FIFO order.

But from looking at the code, later if we can process firings quickly again, we'll keep firing events as fast as possible until mTimeout "catches up" to "now".
That seems pretty broken, so I won't flip the precise pref for now.

Comment 11

[Benjamin Smedberg]

Comment on attachment 466534 [details] [diff] [review]
Add prefs

ok. It sounds like we want some kind of "precise-skipping" timer.

Comment 12

[Boris Zbarsky [:bzbarsky]]

For what it's worth, using a pref cache instead of calling the getter every time might be better, esp. in the e10s world.  But that can be a followup bug...

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I sincerely hope in the e10s world each process gets at least a read-only copy of the prefs so every pref read isn't IPC...

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/c48266f6fb10

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is probably responsible for a small Dromaeo DOM regression. When mozilla-central is not too busy, I'll try temporarily checking in a patch to lower the refresh rate to 50Hz again to check if the change in refresh rate is the cause of the regression.