1354080 - Excessive CPU usage while the connecting/loading spinners are showing

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Jonathan Kew [:jfkthame]]

From bug 759252:
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #104)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #103)
> > Do we care about the power usage regression that this will cause?
> 

> If you're asking about only when it is visible, it is a more nuanced
> question since it won't be visible for the whole time that Firefox is
> running. At this point, is it large enough of an issue to prioritize it?

I've been noticing this repeatedly in Nightly during the last few days: when the connecting/loading throbber is running, Nightly's CPU usage goes through the roof -- in Activity Monitor on macOS, the %CPU utilization of the main Nightly process goes from a "resting" value of < 10% to somewhere in the 150-200% range. (Note that this is separate from the CPU usage of the content process that's actually loading the page; AFAICS, this is basically just due to the chrome process animating the throbber.)

Moreover, if the throbber remains active for an extended period (e.g. several seconds trying to initiate a connection on slow/flaky wifi, or just a very slow-loading page) the system's fans start to run pretty high.

IMO, this isn't a good user experience; we need to either figure out how to reduce the CPU (and hence power) usage of these throbbers, or revert the theme change that introduced them.

Comment 1

[Jonathan Kew [:jfkthame]]

Here's a relevant profile: https://perfht.ml/2nHtjai.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

An approach very similar to this, using a sliding SVG filmstrip to generate animations, is planned for Photon work. It's important that this gets attention ASAP so that we can shift our strategy for implementing the animations if this route will not be sustainable. Marking [qf] in the whiteboard to get this on to the Quantum Flow teams triage.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Looked at the profile today with jrmuizel. We've got two questions, which might be the subject of two separate bugs here:

1) Why are we calling the refresh driver and TabParent::DidRefresh so often?

2) Is it necessary for each refresh driver tick to be so expensive inside TabParent::GetChildProcessOffset?

Hey mattwoodrow - any ideas for (1)? Is it expected that spinning the tab throbbers at 60fps on the compositor will cause such frequent ticks of the refresh driver?

For kats - any ideas for (2)? hg blame suggests that TabParent::DidRefresh and friends are things you wrote. Is there any low-hanging fruit in there for making DidRefresh cheaper?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

And to echo what jaws said in comment 2, a sizeable chunk of the Photon project aims to move as much of our animation off of the main thread and onto the compositor as possible. We should figure out what's going on here as soon as possible.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Mike Conley (:mconley) from comment #3)
> 2) Is it necessary for each refresh driver tick to be so expensive inside
> TabParent::GetChildProcessOffset?
> 
> For kats - any ideas for (2)? hg blame suggests that TabParent::DidRefresh
> and friends are things you wrote. Is there any low-hanging fruit in there
> for making DidRefresh cheaper?

So, I did add the TabParent::DidRefresh code, back in bug 1153023. The reason it was needed was because in Gaia there was code that would set CSS transforms on the child browser area, and we needed to propagate that to the child process via the UpdateDimensions IPC message. However there was no event we could listen for on the parent side to find out when the CSS transform changed, so we just did on every refresh. Calling GetChildProcessOffset wasn't supposed to be that expensive.

That being said, I see two options on how to fix this:
(1) remove the DidRefresh entirely. If firefox never sets CSS transforms on the content browser windows (I suspect we only did this on B2G, and don't do it on desktop/mobile) then this code will never actually get exercised and it can be removed. I don't know if there are add-ons that might do this though.
(2) Try to optimize the GetChildProcessOffset code somehow. It seems like most of the time is being spent in a QI call which I find kind of odd. I'm not sure which QI call it is but I feel like pretty much none of the code in GetChildProcessOffset should require a nontrivial QI.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (1) remove the DidRefresh entirely. If firefox never sets CSS transforms on
> the content browser windows (I suspect we only did this on B2G, and don't do
> it on desktop/mobile) then this code will never actually get exercised and
> it can be removed. I don't know if there are add-ons that might do this
> though.

Yes, pretty sure we don't do this, and add-ons doing it would break anyway as we drop support for XUL add-ons.

Comment 7

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #6)
> (In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> > (1) remove the DidRefresh entirely. If firefox never sets CSS transforms on
> > the content browser windows (I suspect we only did this on B2G, and don't do
> > it on desktop/mobile) then this code will never actually get exercised and
> > it can be removed. I don't know if there are add-ons that might do this
> > though.
> 
> Yes, pretty sure we don't do this, and add-ons doing it would break anyway
> as we drop support for XUL add-ons.

Concur on desktop... does anyone know about Android?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Gijs from comment #7)
> Concur on desktop... does anyone know about Android?

Paging sebastian

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Actually we don't even have e10s enabled on Android, so it's a moot point. Feel free to rip it out.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Okay great, we can get rid of DidRefresh, and that'll help here.

But the other question remains: is it necessary for us to be ticking the refresh driver so often in the parent process when doing this kind of animation? Hoping mattwoodrow can weigh in on that one.

Comment 11

[Markus Stange [:mstange]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (1) remove the DidRefresh entirely. If firefox never sets CSS transforms on
> the content browser windows (I suspect we only did this on B2G, and don't do
> it on desktop/mobile) then this code will never actually get exercised and
> it can be removed. I don't know if there are add-ons that might do this
> though.

I think this is what we should do. I also think that the child process offset should not respect transforms, and pretend the browser element is at the non-transformed position - otherwise we could get into an inconsistent state without this updating mechanism in case somebody does end up transforming a remote browser element. But if we don't respect the transform, then we don't need to update any values when it changes.

Events that are sent to the child process should go through the transforms on the parent process side. And screen-relative coordinates inside the child process should be the ones they'd be if there was no transform.

Comment 12

[Matt Woodrow (:mattwoodrow)]

The refresh driver ticking at 60fps is the expected behaviour (for a foreground tab/the parent process).

We still want to notify the animation system that time is progressing, but we expect it to bail out fairly quickly and not do any real work if all the present animations are running on the compositor.

This looks like it's working correctly as we see some time being spent in 'CanThrottle' underneath the DocumentTimeline::WillRefresh part.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1354080 - Stop doing needless work in the parent process every time the refresh driver ticks.

When we're animating, we tick the refresh driver. If that occurs in the parent process
when e10s is enabled, then we currently run TabParent::DidRefresh which does some
dimensions calculations and might send a message to the content process if the
dimensions have changed.

This was originally added to fix a B2G bug in bug 1153023. We don't need to do it
anymore, since we don't set CSS transforms on content browser windows.

Review commit: https://reviewboard.mozilla.org/r/127374/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127374/

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8855541 [details]
Bug 1354080 - Stop doing needless work in the parent process every time the refresh driver ticks.

https://reviewboard.mozilla.org/r/127374/#review130086

Make sure to do a try push before landing!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b43dc3ad001
Stop doing needless work in the parent process every time the refresh driver ticks. r=kats

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #14)
> 
> Make sure to do a try push before landing!

Cripes. I did not see that part. :/ Whoops. Well, push looks green so far - hopefully I've dodged a bullet.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b43dc3ad001

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b43dc3ad001

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Hey jfkthame, are you able to test tomorrow's Nightly (or a local build with the above revision) and see if CPU usage has improved?

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Mike Conley (:mconley) from comment #19)
> Hey jfkthame, are you able to test tomorrow's Nightly (or a local build with
> the above revision) and see if CPU usage has improved?

Yes, it seems considerably improved... looks like we're back to around 130% CPU usage for the chrome process while the spinner is showing. (That still seems like a lot of CPU just to keep a small spinner moving, but at least it's no longer a big regression from where we were previously.)

Comment 21

[Dão Gottwald [:dao]]

No improvement according to tp5o % Processor Time though?

https://treeherder.mozilla.org/perf.html#/graphs?timerange=2592000&series=%5Bautoland,e8ce194c0d9e4e7986180a313ea7e692fe2182b0,1%5D&series=%5Bmozilla-inbound,e8ce194c0d9e4e7986180a313ea7e692fe2182b0,0%5D&selected=%5Bautoland,e8ce194c0d9e4e7986180a313ea7e692fe2182b0%5D

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Hey mattwoodrow, would you consider the remaining tp5o % Processor Time regression is likely us waking up the main thread more often when we tick the refresh driver with our 60fps throbber?

Comment 23

[Matt Woodrow (:mattwoodrow)]

The main-thread wakeup rate should be the same. The higher framerate on the compositor will contribute to more processor time for sure though.

Comment 24

[(no longer active)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> The main-thread wakeup rate should be the same. The higher framerate on the
> compositor will contribute to more processor time for sure though.

Is there a bug tracking fixing the remaining regression?

Comment 25

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #24)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #23)
> > The main-thread wakeup rate should be the same. The higher framerate on the
> > compositor will contribute to more processor time for sure though.
> 
> Is there a bug tracking fixing the remaining regression?

I filed bug 1357093 for this, and the other Talos regressions from the new throbber animations.