1778575 - If requestAnimationFrame misses the 16.6ms budget, we fall back to 40 fps

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Markus Stange [:mstange]]

Profile: https://share.firefox.dev/3O4YLvH

Testcase: https://jrmuizel.github.io/dl-perf/sommerset-js-work.html?spin=18

If a requestAnimationFrame handler takes long enough that we can't manage to run it every vsync, we end up getting into a tick/tick/skip pattern. For example, when vsync ticks at 60fps, this means that we go down to 40fps if the tick takes longer than 16.6ms.

You might argue that, if we're consistently missing 60fps, we should fall down to 30fps to at least get even frame pacing. Or we should go as fast as we can. So either 30fps or 57fps might be an acceptable solution. But the 40fps behavior that we have now is the worst of both worlds.

It looks like we're the only browser with this kind of behavior. Both Chrome and Safari seem to be willing to call requestAnimationFrame callbacks at any time during a frame, and keep the CPU completely busy without any gaps. So they've picked the "57fps" behavior.

Olli, how do you feel about "unlocking" the start time of the refresh tick, in the case where we can't keep up with the vsync rate? I think this will undo from the work from (the amazingly named) bug 1371668.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1371668

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Sounds reasonable, but better to test that the change doesn't regress high frequency handling (unlikely) nor bug 1755006 (perhaps a bit likelier, but shouldn't, since mSkippedPaints is handled at the nsRefreshDriver level, not RefreshDriverTimer).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I wonder if we could add a test for this once this is fixed. Regressing scheduling behavior is easy, since there aren't too many tests... because writing tests can be tricky.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1371668

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Markus, do you plan to fix this?

Comment 6

[Markus Stange [:mstange]]

I'll be busy with profiler work for the rest of the year, probably, so I won't have time for this in the near future.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Need to ensure bug 1755006 doesn't regress when fixing this.

And the priority of DidComposite has changed since bug 1371668 landed - it is now using control priority (and JS shouldn't run in such tasks).

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: slowraf.html

Do we actually have an issue, or in which case exactly?
Since DidComposite has the highest possible priority, if rAF takes lots of time and there is a pending DidComposite, DidComposite will be processes ASAP after rAF and then a pending vsync can be handled again.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: slowraf.html (v2, with painting every rAF)

Ok, this one shows the difference

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1778575, try to run a pending vsync asap if the main thread is otherwise empty, or if we skipped paints because of pending transactions, r=mstange,bas

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1778575, add support for low priority tasks and use such to trigger pending RefreshDriverTimer, r=bas

This improves the setup so that whenever there are only idle/deferred tasks, we can run low priority tasks immediately.
Manual testing shows that this way we get couple of more ticks every now and then.

Without this patch we'd rely on normal priority tasks, but since there might be multiple of those pending, ShouldGiveNonVsyncTasksMoreTime() would
return true.

So, low priority tasks are effectively "run when the queue is otherwise empty (but idle-type of tasks are still special)".

Depends on D159260

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6721a4116d17
try to run a pending vsync asap if the main thread is otherwise empty, or if we skipped paints because of pending transactions, r=bas
https://hg.mozilla.org/integration/autoland/rev/74c85cc921c1
add support for low priority tasks and use such to trigger pending RefreshDriverTimer, r=bas

Comment 13

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/6721a4116d17
https://hg.mozilla.org/mozilla-central/rev/74c85cc921c1

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 15

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #35702 (as of Thu, 20 Oct 2022 01:57:34 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
7%	motionmark-htmlsuite	linux1804-64-shippable-qr	fission webrender	57.03 -> 60.88
7%	motionmark-animometer	linux1804-64-shippable-qr	fission webrender	48.38 -> 51.55

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=35702