1373343 - stylo: Only do parallel traversals when the tab is foreground

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P2)

Description

[Bobby Holley (:bholley)]

There have been some discussions around the desired interaction between stylo and the smart scheduling we're building for e10s-multi and quantum DOM.

The basic issue is that the parallel stylo traversal is optimized to used all the resources of the machine to style the page as fast as possible (we use a threadpool sized to num_virtual_cpus * (3/4)), which basically (assuming hyperthreads) means 3 threads on a 2-core and 6 threads on a 4-core machine. We may end up tweaking these a bit, but that's the general idea.

There's only one stylo threadpool per process, and styling only happens on the main thread [1]. However, in an e10s-multi world, we could potentially have a number of these threadpools, and it seems counter-productive to have them competing with each other.

So the obvious thing, to me, seem to be that we should only allow parallel styling when the document being styled is in the foreground. That should limit things to a single content process (at least until process-per-iframe), and also satisfies the heuristic that, if a tab is in the background, we probably don't want to devote every ounce of system resources to styling it faster.

The easiest way to do this is to have a bit/bool on the document that indicates whether the given document is in the foreground or not. Ideally somebody familiar with this stuff would set up this bool and/or point me to it, at which point I can modify stylo appropriately.

[1] Though I'm not exactly sure what this means in a Quantum DOM world?

Comment 1

[Bobby Holley (:bholley)]

Does this sound reasonable to everyone? If so, gabor, can you (or someone on your team) point me to the bit/bool I should target?

Comment 2

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

I think you should target nsIPresShell::IsActive().  That corresponds to things like whether we've throttled back the refresh driver, and it should be true for foreground tabs and false for background ones.

Comment 3

[Bobby Holley (:bholley)]

That works for me. Gabor and bill, can you confirm that this is desirable from the e10s-multi and quantum DOM perspectives?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, this sounds like the right approach to me. If it's easier for you, there's a similar flag on the docshell.

Comment 5

[Chris Peterson [:cpeterson]]

Note that there may be multiple foreground tabs if the user has multiple windows open, e.g. on multiple monitors. Does Quantum DOM prioritize the foreground tab of the focused window (if one actually has focus!) over the foreground tab of unfocused windows? They are all "foreground" tabs, but the user's attention is presumably on the focused window's foreground tab so we may be able to deprioritize the unfocused foreground tab without the user noticing.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Chris Peterson [:cpeterson] from comment #5)
> Note that there may be multiple foreground tabs if the user has multiple
> windows open, e.g. on multiple monitors. Does Quantum DOM prioritize the
> foreground tab of the focused window (if one actually has focus!) over the
> foreground tab of unfocused windows? They are all "foreground" tabs, but the
> user's attention is presumably on the focused window's foreground tab so we
> may be able to deprioritize the unfocused foreground tab without the user
> noticing.

No, all foreground tabs are treated equally. Without knowledge of how much a window is obscured, I think it's dangerous to prioritize one foreground tab over another. Even if a window is in the background, the user might still be focused on it (on another monitor or something).

We've talked about trying to get information about how windows overlap, but it's complicated and I'm not sure it's worth it. 80% of users have only one window open at any time, and 95% have one or two.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #3)
> That works for me. Gabor and bill, can you confirm that this is desirable
> from the e10s-multi and quantum DOM perspectives?

Thanks for filing this bug, and yes this sounds great. It might make sense to try it out in practice some time, what happens in the two concurrent foreground window case (different processes). I wouldn't try to optimize that scenario just check if the browser or OS won't start to jank heavily if we have too many threads active. Someone from QA at some point could spend an hour or two on testing this, but I bet if this is on on nightly we will have bugs filed about it if something goes terribly wrong anyway.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Skip the parallel traversal when the presshell isn't active. v1

I've verified with the profiler that we use the parallel traversal for
loading a foreground tab, but not for a tab opened with ctrl-click.

MozReview-Commit-ID: 2SiVDlLLyah

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8898360 [details] [diff] [review]
Skip the parallel traversal when the presshell isn't active. v1

Review of attachment 8898360 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleSet.cpp
@@ +878,5 @@
>      // Allow the parallel traversal, unless we're traversing traversing one of
>      // the native anonymous document style roots, which are tiny and not worth
>      // parallelizing over.
> +    //
> +    // We only allow the parallel traversal in active (forground) tabs.

foreground

@@ +983,5 @@
>    // traversals in this case to preserve the old behavior (where Servo would
>    // use the parallel traversal i.f.f. the traversal root was the document root).
> +  if (aParent == aParent->OwnerDoc()->GetRootElement() &&
> +      mPresContext->PresShell()->IsActive())
> +  {

Nit: "{" should still go on the previous line.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f32be31f0998
Skip the parallel traversal when the presshell isn't active. r=heycam

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f32be31f0998