Remove perf-adaptive layout.frame_rate code

RESOLVED FIXED in Firefox 68

Status

()

enhancement
P1
normal
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: mstange, Assigned: Gijs)

Tracking

Trunk
Firefox 68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

(Whiteboard: [fxperf:p1])

Attachments

(1 attachment)

Reporter

Description

5 months ago

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.

Assignee

Comment 1

5 months ago

(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" ?

Flags: needinfo?(mstange)
Reporter

Comment 2

5 months ago

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.

Flags: needinfo?(mstange)

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

Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [fxperf]
Assignee

Comment 4

5 months ago

I'll take this.

Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(gijskruitbosch+bugs)
Priority: -- → P1
Whiteboard: [fxperf] → [fxperf:p1]
Assignee

Comment 5

5 months ago

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!

Flags: needinfo?(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

Flags: needinfo?(botond)
Assignee

Updated

4 months ago
See Also: → 1523838
Assignee

Comment 7

3 months ago

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.

Summary: Adaptive lowering of the frame rate affects scrolling frame rate → Remove perf-adaptive layout.frame_rate code

Comment 10

3 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8a3793bf9c42
remove nightly-only low-end device detection, r=kats,chutten,flod,mconley

Comment 11

3 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.