1519241 - Remove perf-adaptive layout.frame_rate code

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Markus Stange [:mstange]]

The patch in bug 1503339 lowers the global vsync rate to 30Hz if Firefox is running on a low power device. The vsync rate affects scheduling both of the refresh driver, and of compositing. As a consequence, this patch also affects the frame rate of scrolling.

It would be nice to find a solution which dials down the main thread refresh driver rate, and maybe also the OMTA sampling rate (e.g. so that the tab throbber animation is only sampled at 30fps), but keeps scrolling at a responsive 60 fps.

Comment 1

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #0)

The patch in bug 1503339 lowers the global vsync rate to 30Hz if Firefox is running on a low power device. The vsync rate affects scheduling both of the refresh driver, and of compositing

Does it affect anything else?

Would the long term plan here be best done as "make the global vsync rate lower, but expose some other rate that the compositor can use" or "keep the global vsync rate the same but expose something else for the refresh driver, OMTA, and any other consumers we feel comfortable throttling" ?

Comment 2

[Markus Stange [:mstange]]

I think I'd prefer the latter. If we exposed some other rate to the compositor that's not vsync, I think it'd be hard to make scrolling smooth and uniform (i.e. aligned with vsync).

For example, instead of using a software timer, we could still listen for regular vsync and then do the following:

• In the refresh driver, when we get a vsync notification, if we should throttle and less than 30ms have elapsed since the previous handled vsync notification, ignore this notification and listen for the next.
• In the compositor, do the same unless there is a pending APZ scroll update. If there is a pending APZ scroll update, composite on the vsync regardless of how much time has elapsed.

With that solution, the compositor thread will still wake up at the same rate, but it'll do nothing around half the time. We'd have to measure whether this approach comes with the same power savings.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Gijs, could you please guess a priority for this one?

Comment 4

[:Gijs (he/him)]

I'll take this.

Comment 5

[:Gijs (he/him)]

Botond, what would be the best way to hook into scrolling start/end notifications in order to temporarily alter frame rates across the board? Markus has already pointed out https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/gfx/layers/apz/src/AsyncPanZoomController.cpp#4809-4810,4832-4833 but that (per :mstange) runs in the gpu process on the apz controller thread, and I expect we'll need to change this on the main thread in the parent process for things to "work" everywhere and propagate correctly... some pointers would be really helpful!

Comment 6

[Botond Ballo [:botond]]

The notifications you mentioned are sent to the main thread and arrive here.

However, they are sent to the main thread of the process rendering the content that is (or was) scrolling, which in most cases is a content process, not the parent process.

If you want to know about the start/end of all scrolling in the parent process main thread, we probably need a new notification for that. NotifyPinchGesture [1] is an example of a notification that is always sent to the parent process, you could add a similar one to GeckoContentController for this purpose and dispatch it from the same place that you linked to.

Let me know if that helps / if you'd like me to go into more detail.

[1] https://searchfox.org/mozilla-central/rev/330daedbeac2bba296d663668e0e0cf248bc6823/gfx/layers/apz/public/GeckoContentController.h#89

Comment 7

[:Gijs (he/him)]

The outcome of a lot of measuring here is that ultimately, this doesn't buy us anything at the moment (esp. not after bug 1506949 and friends), and so I'm going to remove the code to avoid it hanging around and confusing people.

Comment 8

[:Gijs (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a519eed686217628c173282745b2fd73e6e5cb59

Comment 9

[:Gijs (he/him)]

Attachment: Bug 1519241 - remove nightly-only low-end device detection, r?kats!,chutten!,flod!,mconley!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a3793bf9c42
remove nightly-only low-end device detection, r=kats,chutten,flod,mconley

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8a3793bf9c42