1308660 - for background tabs, break refresh driver tick into multiple events off the main thread's event loop

Status: NEW

(Core :: Layout, enhancement)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

A few weeks ago I was discussing with smaug the problem of background tabs sometimes hogging the main event loop for an extended period of time, sometimes in layout.

Since we're running refresh ticks in background tabs quite infrequently (1 second, I think), a decent amount of work can accumulate by the time we get around to ticking the refresh driver, particularly during page loading.

One thing that occurred to me is that, for background tabs, we don't need to run the entire refresh tick in a single event off the main event lop.  Instead, we could split it up to run in multiple events off the main event loop, to reduce the amount of time that it keeps us away from the event loop.

There are two different approaches to the splitting of work for a single page:

  Approach A would be something roughly like:
    - on event 1, run everything through content flushing
    - on event 2, run everything through style flushing / frame construction
    - on event 3, run everything through reflow
  That is, later events would "duplicate" the work of the earlier ones,
  but hopefully there wouldn't be anything or much accumulated to do.

  Approach B would actually be to split up the refresh driver tick without
  duplication.  This *might* require that we ensure nothing else from the
  page happens in between, which we don't currently have the ability to do,
  but which is being worked on.  This would make it safer to do finer-grained
  splitting (which I'll file an additional bug on some ideas for).

Additionally, we should also be more aggressive about splitting up the work between a parent document and its subdocuments.  I *believe* these currently share the same refresh driver, which I means we're currently running the refresh tick for all these subdocuments on a single event off of the main thread.  We should instead separate these to different main thread events, with ancestor documents before descendant documents (since flushing in a descendant can cause flushing in an ancestor).

Comment 1

[Boris Zbarsky [:bzbarsky]]

> I *believe* these currently share the same refresh driver

Do all background tabs also share a single refresh driver?  And if so, should we break these up too, presumably?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think so, based on a quick reading of nsPresContext::Init.  I was just saying that parent documents and their subdocuments (iframes) share the same refresh driver.

Comment 3

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 4

[Jet Villegas (inactive)]

A RefreshDriver overhaul like described here is a multi-month job. I've seen other risky background tab optimization requests pop up with qf:p1 designations. Layout team is focused on the front tab for Quantum Flow.

Comment 5

[Naveed Ihsanullah [:naveed]]

Will our work on Quantum DOM makes this issue less?

Comment 6

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

In theory we could preempt between different phases of a tick, I think.

Comment 7

[Jet Villegas (inactive)]

(In reply to Naveed Ihsanullah [:naveed] from comment #5)
> Will our work on Quantum DOM makes this issue less?

I think this is what dbaron means is a prerequisite for what he calls Approach B above. Can Quantum DOM guarantee that content won't need to flush again on a subsequent tick when Layout flushes?

Comment 8

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

If preemption gets implemented properly, we should be able to guarantee flush isn't needed.
But to be more precise, this wouldn't be about all background tabs, but background tabs in a certain tabgroup. If say foreground tab A and a background tab B share the tabgroup, QDOM can't help here
(assuming foreground tab is kept running all the time).

Comment 9

[(no longer active)]

How about pursuing approach A?  It seems not too hard to do.  Is the risk of duplicating work too bad in practice?

Comment 10

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

I was looking for a testcase for this (since reflow is usually by far the slowest part here) and looks like loading single page HTML spec in background works as such. I can see processing pending styles taking 25% of ticks, reflow 75%.

And yes, A at least doesn't sound too hard to do. In fact it sounds like simple enough that we could try it to see how it works or whether it creates some issues.

Comment 11

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

I think splitting the work to two is probably enough, first event does style flush, second style+layout.

Comment 12

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

Hmm, animation events and such might broke easily with A.

Comment 13

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

Attachment: quick patch for testing

Or maybe it could be something as simple as this, since styles can be flushed anyhow before dispatching any events. So animation events etc. will be dispatched when doing the real tick.

This drops % style flushing takes during RefreshDriver::Tick from 25% to 4% in the "single page HTML spec in background" case, so the actual Tick becomes almost only reflow and style flushing happens async beforehand.

Comment 14

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

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

Comment 15

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

Without the patch first tick with single page HTML spec takes ~740ms and with the patch ~560ms (and time is spent async before tick doing style flush).

Profiler reports about the same delay for event processing, but I guess that depends on how it is measured, since clearly if some work is split to two pieces, other runnables may be processed between them.

Comment 16

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

Comment on attachment 8863220 [details] [diff] [review]
quick patch for testing

dbaron, what you think of this kind of super simple approach?

Comment 17

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

And the patch works the way it does, and not doing reflow-less-but-otherwise-normal-tick first and then reflow only tick, since it isn't clear how nsARefreshObservers would work in that kind of setup.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8863220 [details] [diff] [review]
quick patch for testing

So I'm not happy about the code duplication -- I worry we'll pile more stuff on top and then be unable to unduplicate.  I'd much rather start with splitting up of the refresh driver's tick method.

I'm fine with starting off with splitting out a "restyling part", and then trying to do some ticks that just do the restyling part and some ticks (nearly immediately after) that do (I guess) everything, including flushing a hopefully-empty restyle queue.

Maybe we (dbaron, smaug, bz) should sit down this week to talk.

(Also, I still have no idea what to do with a feedback?, so just clearing it.)

Comment 19

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

Yeah, I could have a helper method to go through style observers.
I'll prepare a patch for that.

Comment 20

[Andrew Overholt [:overholt]]

Olli said we could probably make this p2.

Comment 21

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

I reconsidered this. I think this should be p1. Creating frames is slow, and then we do also slow reflow, so better to try to do that all in two or more steps when dealing with background tabs.

Comment 22

[Bobby Holley (:bholley)]

smaug, would this still be useful in a more-e10s world?

Comment 23

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

Yes, this or the variant where we don't run refreshdriver at all in the background.

Comment 24

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

...because styling and reflow are after all rather slow, and quite often several tabs will be in the same docgroup even in more-e10s world.

Comment 25

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

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

Comment 26

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

This should be qf:responsiveness:p<x> .
Possibly because of Fission this will become more important if user has tabs using same process.

One thing we should probably do here is to wait for 1s and then post a runnable to the idle queue to ensure we less likely run
RefreshDriver in background tabs when there is something more important happening.

Comment 27

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

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 28

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.