Open Bug 1352205 Opened 7 years ago Updated 1 year ago

Avoid running the refresh driver when the page is in background

Categories

(Core :: Layout, enhancement, P2)

enhancement

Tracking

()

Performance Impact low

People

(Reporter: ehsan.akhgari, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, perf:resource-use)

Attachments

(5 files)

Apparently Chrome has stopped doing this a long time ago.
Blocks: QuantumFlow
So requestAnimationFrame() won't run on background tabs?
Whiteboard: [qf]
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #1)
> So requestAnimationFrame() won't run on background tabs?

Correct.
Whiteboard: [qf] → [qf:p1]
Per comment 2, we already throttle the refresh driver (and dependencies like RaF) to 1Hz when hidden or in a background tab. 

Chromium based their implementation on ours but use the IntersectionObserver cross-origin rules to determine if they can completely skip animations for hidden ads:

https://codereview.chromium.org/1364063007/diff/240001/third_party/WebKit/Source/core/frame/FrameView.cpp

I'd prefer to ship IntersectionObserver first before we hang other features on it. It also seems like we'd get better perf since we already throttle hidden content (ads or not) to 1Hz. It may be possible to slow down the refresh driver tick even further for hidden content, though we got to once per second after some debate.
See Also: → 1357843
Is this bug really worth rating as [qf:p1], given comment 2?

Unless we have known issues from 1-RAF-tick-per-second background tabs hogging CPU during that 1 tick, I'm not sure there's much here that's immediately actionable or worth throwing resources at, right now.
Flags: needinfo?(ehsan)
(In reply to Jet Villegas (:jet) from comment #4)
> Chromium based their implementation on ours but use the IntersectionObserver
> cross-origin rules to determine if they can completely skip animations for
> hidden ads:
> 
> https://codereview.chromium.org/1364063007/diff/240001/third_party/WebKit/
> Source/core/frame/FrameView.cpp
> 
> I'd prefer to ship IntersectionObserver first before we hang other features
> on it. It also seems like we'd get better perf since we already throttle
> hidden content (ads or not) to 1Hz. It may be possible to slow down the
> refresh driver tick even further for hidden content, though we got to once
> per second after some debate.

https://codereview.chromium.org/1364063007 where this code that you linked to comes from is implementing this proposal <https://docs.google.com/document/d/1Dd4qi1b_iX-OCZpelvXxizjq6dDJ76XNtk37SZEoTYQ/edit#>.  As far as I can tell, that is a completely different thing that what this bug is on file for, that's a proposal for suspending the refresh driver in cross origin iframes that are not in the viewport on foreground pages, sort of along the lines of bug 1357843 but not exactly the same as that.
(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #5)
> Is this bug really worth rating as [qf:p1], given comment 2?

I'd say yes, since running the refresh driver involves running scripts from pages which can be arbitrarily expensive.  Also running stuff off of timers is terrible for use cases such as being responsive to user input events.  For one thing this needs to be ported to use idle dispatch.

We're planning things like Quantum DOM which will help some but they won't help with cases where the background page is in the same tab group as the foreground page or for other parts of the refresh driver tick besides running the rAF callbacks.

I think if we ported this to use idle dispatch, then maybe what InactiveRefreshDriverTimer currently does is good enough, but I'm not sure I understand exactly what cases can cause its timer to be reset currently, and its effectiveness really depends on that.

Still I think this bug is fairly high priority, at least for as long as we have a timer that can come around and jank at exactly the wrong time.

> Unless we have known issues from 1-RAF-tick-per-second background tabs
> hogging CPU during that 1 tick, I'm not sure there's much here that's
> immediately actionable or worth throwing resources at, right now.

Let's separate out the issues of lack of human resources with things not being actionable.  What can we do to make the bug more actionable?

We have known issues from pages hogging the CPU while running scripts, running restyles and reflows.  Do we need more evidence to believe that this is a serious problem that we need to address?
Depends on: 1358476
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #7)
> Let's separate out the issues of lack of human resources with things not
> being actionable.  What can we do to make the bug more actionable?

RE this being actionable: it would help to have an example or two of an actual live site that triggers this pathological background-tab behavior, so that we can evaluate different approaches on real world content.  In the absence of real-world testcases, this all feels kinda theoretical & it's hard to decide which strategy/ies to invest in & to evaluate what sort of wins we can expect.

Right now we've had several suggested approaches here:
 - Waiting for IntersectionObserver to ship & seeing how that helps (it will presumably do away with some "am I visible" rAF ticks from ads in background tabs, for ads/sites that play nicely & use this feature).
 - Porting refresh driver to use idle dispatch. [I admit I don't fully know what this means but it sounds good]
 - Reducing the baseline refresh driver frequency below 1hz to some arbitrary lower value, so scripts still run but very infrequently.
 - Reducing the baseline refresh driver frequency to 0 (as originally suggested here)

There are different pros/cons (including development time & potential to break the web) for each of these approaches, so it's hard to decide on which approach(es) to pursue & invest in, in the absense of specific real-world use cases.

> We have known issues from pages hogging the CPU while running scripts,
> running restyles and reflows.  Do we need more evidence to believe that this
> is a serious problem that we need to address?

Of course not. But what you've described there is largely the whole of quantum flow. :) I'm trying to understand the scope of what in particular we want to solve on this specific bug.
(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #8)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #7)
> > Let's separate out the issues of lack of human resources with things not
> > being actionable.  What can we do to make the bug more actionable?
> 
> RE this being actionable: it would help to have an example or two of an
> actual live site that triggers this pathological background-tab behavior, so
> that we can evaluate different approaches on real world content.

I personally feel like I'm still missing something here.  See this profile: https://perfht.ml/2p74VzP  I just grabbed it from my normal browsing profile without really doing anything special, and I searched for "InactiveRefreshDriverTimer::TickOne" there.  As you can see, I do have refresh drivers for background pages in all of my content processes pretty much running all the time.  They sometimes seem to run every second, sometimes every 2-3 seconds, it's hard to see a pattern just by looking at the profiles...   (I think this may be because I was switching tabs back and forth, etc?)

>  In the
> absence of real-world testcases, this all feels kinda theoretical & it's
> hard to decide which strategy/ies to invest in & to evaluate what sort of
> wins we can expect.

This bug was originally filed because someone (I sadly forget who) mentioned during the QF work week that Chrome had stopped dispatching rAF callbacks a while ago.  I was hoping that someone would investigate what Chrome (and other browsers) are doing for background pages today.  It certainly seems to me that any work that we do to dispatch rAF callbacks and running the rest of the refresh driver tick for background pages is wasted work if we do it more frequently than Chrome/other browsers do.

In terms of test cases, I'm not sure what types of test cases you're looking for.  I expect you're hoping for a real website that runs an expensive refresh driver tick?  Here is a profile that I just captured by loading nytimes.com, foxnews.com and cnn.com in three tabs on the reference hardware and it shows a 48ms jank caused by the nytimes.com tab: https://perfht.ml/2qtNq1s.  Gathering similar profiles is really as simple as loading vious pages and putting them in background tabs and profiling.

Do these profiles help?  If not, maybe it would be better if someone who's looking to answer some specific question to do more targeted profiling?

> Right now we've had several suggested approaches here:
>  - Waiting for IntersectionObserver to ship & seeing how that helps (it will
> presumably do away with some "am I visible" rAF ticks from ads in background
> tabs, for ads/sites that play nicely & use this feature).

I still think IntersectionObserver is orthogonal.  Here is a simple example.  Consider this case.  Tab A is sitting in the background, and there are 0 ads on it, and it doesn't use IntersectionObserver.  It sets a timeout.  The timeout handler runs, and it changes the style of some element.  Then the inactive refresh driver will come around at some point and run a tick and flushes styles etc.

>  - Porting refresh driver to use idle dispatch. [I admit I don't fully know
> what this means but it sounds good]

It basically means not running the inactive refresh driver at times when the user is interacting with the browser.  Once bug 
1358476 gets fixed this should be as simple as porting the inactive refresh driver timer to use the API being worked on there instead of nsITimer for the most part (aka not a lot of work.)

>  - Reducing the baseline refresh driver frequency below 1hz to some
> arbitrary lower value, so scripts still run but very infrequently.
>  - Reducing the baseline refresh driver frequency to 0 (as originally
> suggested here)

FTR the original suggestion was to do what Chrome does.  :-)  I never verified that it has actually reduced the frequency to 0.

> There are different pros/cons (including development time & potential to
> break the web) for each of these approaches, so it's hard to decide on which
> approach(es) to pursue & invest in, in the absense of specific real-world
> use cases.

OK, fair enough.  This isn't intended to solve one specific test case, it is intended to reduce the amount of work that the content process main thread can potentially be expected to do on behalf of a page in that content process that is sitting in the background, to ensure that the main thread will be as free as possible to service a page in the content process in the foreground.  That should be a specific use case if not "one" specific test case.  :-)

> > We have known issues from pages hogging the CPU while running scripts,
> > running restyles and reflows.  Do we need more evidence to believe that this
> > is a serious problem that we need to address?
> 
> Of course not. But what you've described there is largely the whole of
> quantum flow. :) I'm trying to understand the scope of what in particular we
> want to solve on this specific bug.

I do hope this helps clarify the goal that I'm pursuing in this bug.  :-)
See Also: → 1308660
See also bug 646937: throttle mozRequestAnimationFrame in completely obscured windows.
See Also: → 646937
The question is whether other browsers suppress just requestAnimationFrame(), or the entire refresh cycle.
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #0)
> [ Avoid running the refresh driver when the page is in background ]
> Apparently Chrome has stopped doing this a long time ago.

For reference -- here^^, I think ehsan was probably recalling this semi-announcement in the Chrome 57 release notes:
# Policies
#   requestAnimationFrame()
#   Per the documentation, Chrome does not call requestAnimationFrame() when
#   a page is in the background. This behavior has been in place since 2011.
https://developers.google.com/web/updates/2017/03/background_tabs#requestanimationframe
Assignee: nobody → tlin
I attached two test cases. rAF-beep.html beeps in requestAnimationFrame() callback and setInterval-beep.html in setInterval(500) callback.  (Warning: lower the volume of the speaker before opening the test cases.)

Loading rAF-beep.html in background tab:
Firefox 55: beeps in 1s, 2s, 4s, ...
Chrome 58: No beep.
Safari 10.1: No beep. (beep for a few seconds still if switching from foreground tab to background tab.)
Edge 40.15063: No beep.

Loading setInterval-beep.html in background tab:
Firefox 55: beeps in 1 Hz
Chrome 58: beeps in 2Hz, same frequency as in the foreground tab.
Safari 10.1: No beep.
Edge 40.15063: beeps in 1Hz, like us.
Based on the test results in comment 15, I feel we could stop calling requestAnimationFrame() for background tabs.
What about testing something that's both (a) not requestAnimationFrame and (b) is connected to the refresh driver (which setInterval is not)?
Flags: needinfo?(tlin)
Add two more tests and the results:

Loading animationiteration-event-beep.html in background tab:
Firefox 55: beeps in 1s, 2s, 4s, ...
Chrome 58: No beep.
Safari 10.1: No beep.
Edge 40.15063: No beep.

Loading smil-beep.html in background tab:
Firefox 55: beeps in 1s, 2s, 4s, ...
Chrome 58: No beep.
Safari 10.1: ? (SMIL is working, but no beep even when it's in foreground.)
Edge 40.15063: ? (SMIL is not working.)
See Also: → 1367830
Whiteboard: [qf:p1] → [qf:p2]
Try is unhappy that I stop the InactiveRefreshDriverTimer from ticking at all.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dee8f18b138bd71d8b1fc63afc672907a6886f1
Boris, you invented the throttle mechanism for the background tabs. Based on those testing results on different browsers (comment 15 and comment 20), does the patch look reasonable to you?

Try results. (The orange should be existing intermittent.)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed6c2979d74536273317beab81ef9eb700a1555
Flags: needinfo?(bzbarsky)
This stuff has changed a bunch since I last looked at it...

If I recall correctly how this stuff works, all background pages in a given process are hooked up to a single InactiveRefreshDriverTimer.  When it ticks it ticks all of them.

Whenever a tab goes into the background, the InactiveRefreshDriverTimer resets to the 1Hz tick right now and starts its backoff again.

With this change, every time a tab goes into the background we would tick _all_ inactive tabs 1s later, then stop.  This is clearly more work than we're doing right now, but why would we want to do even that one tick?  Why not just have InactiveRefreshDriverTimer::ScheduleNextTick always no-op so it never ticks?  Then we can remove a bunch of this machinery altogether.
Flags: needinfo?(bzbarsky)
Status: NEW → ASSIGNED
Priority: -- → P2
Ting-Yu, are you still working on this?  Thanks!
I don't work on this bug this moment.

Here's the latest try result that completely shut down the inactive refresh driver. We might still need to allow some tasks to run at least once to pass the tests, but I haven't got a chance to dig deeper.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea5d34f3ed9977608d0f3560d1131ddc50e9a7f8
Assignee: tlin → nobody
Status: ASSIGNED → NEW
Blocks: 1394447
Keywords: perf
Whiteboard: [qf:p2] → [qf:p1]
Whiteboard: [qf:p1] → [qf:i60][qf:p1]
Whiteboard: [qf:i60][qf:p1] → [qf:f60][qf:p1]
Whiteboard: [qf:f60][qf:p1] → [qf:f61][qf:p1]
This seems like a dupe of bug 1308660 - would you agree, smaug?
Flags: needinfo?(bugs)
IIRC they were different approaches with the same goal (reducing refresh driver load for background tabs).

 - Bug 1308660 is about splitting background-tab refresh driver ticks into smaller pieces.  So with that strategy, requestAnimationFrame() callbacks would still be invoked in these tabs, but there'd be reduced guarantees about the granularity of layout/style flushes being all batched directly together (and IIUC this wouldn't be observable to web content or to users really).

...vs:

 - This bug here (bug 1352205) is about just stopping the refresh driver entirely for background tabs (or something like that. per comment 11).  So requestAnimationFrame() wouldn't run at all in these tabs, per comment 3. (vs. right now we throttle, apparently to once per second.)  This *would be* a web-content-observable change -- though it's also apparently web-compatible because Chrome already does something like this.
Yeah, not a dup of bug 1308660.
Flags: needinfo?(bugs)
Whiteboard: [qf:f61][qf:p1] → [qf:f64][qf:p3]
Whiteboard: [qf:f64][qf:p3] → [qf:p3:f64]
Whiteboard: [qf:p3:f64] → [qf:p3]
See Also: → 1548561

This should probably be retriaged by the qf triage group, and categorized.

Whiteboard: [qf:p3] → [qf]
Whiteboard: [qf] → [qf:p3:resource]
Performance Impact: --- → P3
Whiteboard: [qf:p3:resource]
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 5 See Also bugs.
:dholbert, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(dholbert)
Severity: S3 → --
Type: defect → enhancement
Flags: needinfo?(dholbert)
No longer regressions: power-usage
No longer regressions: 1394447, QuantumFlow
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: