1180916 - [e10s] Switching tab generates 4 frames (4 full runs of the RefreshDriver)

Status: RESOLVED WORKSFORME

(Firefox :: Tabbed Browser, defect, P3)

Description

[Benoit Girard (:BenWa)]

I've gathered several profiles of tab switch times and we seem to be always outputting at least 4 frames in the chrome process. Ideally we need to get this down to 1, bonus points if we can combine that with the present of the content process.

These extra frames are hurting us because:
- While the chrome process is running the refresh driver 4 times, the content process is sending sync messages which are then blocked for up to 15ms.
- Wasted resources.

This is probably a fairly easy way to speed up tab switch times by ~5-45ms.

Comment 1

[Benoit Girard (:BenWa)]

Here's the most interesting profiles I've seen:
http://people.mozilla.org/~bgirard/cleopatra/#report=58a3725a355fbe345a0ec1f0884f7a8117d111a2&filter=[{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A23117266,%22end%22%3A23117474}]&selection=0,2947,2948,2949,2950,2755,2951,8,9,10,11,12,13,14,15,16,17,20,21,22,23,24,25,26,3395,3396,3397,3398,3399,175,3400,3401,3439,3440,3441,3442,3443,3444,3448,3449,3450,3451,82,83,84,85

In this you can see that we have a fairly innocent IPC message 'IPDL::PScreenManager::SendScreenRefresh' which has to wait for the refresh driver for a frame that we probably don't really need to fix.

Let's find out exactly why all these frames are occuring and see if we can combine them into a single frame.

Comment 2

[Benoit Girard (:BenWa)]

Bug 1181297 makes the debugging here a bit easier but not strictly required.

Comment 3

[Benoit Girard (:BenWa)]

Looks like the first extra frame is caused by bug 1181333.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: blassey' paint suppression patch v1

Not the best fix for fix for this. Ideally we would get the code to not paint. But this is probably easier and is less likely to regress as we make changes to the front end code.

I'll get scores for this patch.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: my patch queue, without this patch

Comment 6

[Benoit Girard (:BenWa)]

Attachment: my patch queue, with this patch

Comment 7

[Benoit Girard (:BenWa)]

This patch seems to particularly help test that are already fast (50-80ms). I see them go down by ~10ms-ish millisecond. This is just eyeballing noisy results so I don't really have any confidence metrics. But I'm fairly convinced that we should land this.

Comment 8

[Benoit Girard (:BenWa)]

Given the data above do you plan on getting this patch landed or should we get this an owner?

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

I will

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: globallPaintSuppression.patch

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8646953 [details] [diff] [review]
globallPaintSuppression.patch

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

Looks good, modulo a couple of nits that I noticed.

::: browser/base/content/tabbrowser.xml
@@ +3230,5 @@
>                // loadingTab can be non-null here if we timed out loading the current tab.
>                // In that case we just overwrite it with a different tab; it's had its chance.
> +              if (this.loadingTab == null) {
> +                let dwu = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                 .getInterface(Ci.nsIDOMWindowUtils);

Uber-nit: the second line is misindented.

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1606,5 @@
>     * otherwise.
>     */
>    readonly attribute boolean paintingSuppressed;
>  
> +  void suppressPaintingGlobally();

Please add comments about these two functions.

Also, need to bump the IID here.

Comment 12

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

Comment on attachment 8646953 [details] [diff] [review]
globallPaintSuppression.patch

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

I guess the thing that worries me the most is that the tab switcher object is this pretty complex state machine with async stuff going on, and I worry that we might end up in a state where we haven't done our bookkeeping correctly, and we've suppressed more times than we've unsuppressed, thus preventing the entire DOM window from painting.

I wonder if it'd be safer to have the switcher have some helper methods for suppressing and unsuppressing, that uses its own internal count to increment/decrement. When destroy() is called on the switcher, we should probably ensure that that counter is 0 - otherwise, if > 0, to call unsuppress that number of times to get painting again.

I know that's kinda belt and suspenders, but this tabswitcher kinda freaks me out sometimes.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: globallPaintSuppression.patch

Comment 14

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

Comment on attachment 8647498 [details] [diff] [review]
globallPaintSuppression.patch

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

r=me with the changes below. Thanks blassey!

::: browser/base/content/tabbrowser.xml
@@ +3070,5 @@
>              STATE_LOADING: 1,
>              STATE_LOADED: 2,
>              STATE_UNLOADING: 3,
>  
> +            _suppresser: {

_suppressor

::: dom/interfaces/base/nsIDOMWindowUtils.idl
@@ +1607,5 @@
>     */
>    readonly attribute boolean paintingSuppressed;
>  
>    /**
> +   * Suppress painting by freezing and thawing the refresh driver.

Not freezing and thawing, just freezing.

@@ +1612,5 @@
> +   **/
> +  void suppressPaintingGlobally();
> +
> +  /**
> +   * Unsuppress painting by freezing and thawing the refresh driver.

Not freezing and thawing, just thawing.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2c0405ddf93

Comment 16

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

So I just tested this on an inbound build, and this seems to keep suppressing painting when we should be showing the spinner.

STR:

1) Open 2 tabs and get them to some web property so that both have -e10s in the suffix of their tab tooltips.
2) In one tab, open the Web Console, and type:

while (true) {}

And press Enter

3) Attempt to switch to the other tab

ER:

After about 400 ms we should see the tab spinner.

AR:

No tab spinner. And the notification bar to kill the script will not paint either. We're stuck.

The good news with this patch, however, is that it'll probably fix bug 1151985.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing various e10s mochitest failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=12882223&repo=mozilla-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=12880975&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c7127a6ae54

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Talos e10s timeouts as well.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: globallPaintSuppression.patch

try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=3603687d9fa3

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

I can't reproduce these timeouts locally

Comment 21

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

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #20)
> I can't reproduce these timeouts locally

Which Linux distro were you attempting to reproduce them on?

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mike Conley (:mconley) - (Going dark Aug 20 - Sept 11) from comment #21)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #20)
> > I can't reproduce these timeouts locally
> 
> Which Linux distro were you attempting to reproduce them on?

Ubuntu

Comment 23

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

I've had no success attempting to reproduce this today on my Ubuntu VM either. I've run the entire directory, and the entire bc2 chunk, and I don't get the same test failures with this patch.

However, retriggers on try are pretty consistent.

Not sure what to do here. I think the next step is to get a loaner and try to diagnose it there. Unfortunately, I don't think I'll be able to do that before I PTO out.

I'm also not comfortable just disabling the tests without understanding why they're failing, so I don't think I want to r+ this just yet.

How do you want to play this, blassey?

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

I think either getting a loaner or abusing try with pushes and printfs is the only path forward right now. Unfortunately I don't think I'll be able to get to that for a little bit, so I think this needs to sit in a holding pattern for a while.

Comment 25

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

Comment on attachment 8648133 [details] [diff] [review]
globallPaintSuppression.patch

Okay, minusing for the oranges.

Comment 26

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

I'm actually seeing some of these same failures on an unrelated try build:

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

I wonder if we're both based on something that got eventually backed out?

New try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a841413adbd

Comment 27

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

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

Comment 28

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=297dd1533796

Comment 29

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

Okay, convinced. This patch is breaking something.

Comment 30

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

I was poking at this patch again to see if it could be used to fix bug 1181333, but I'm less sure this patch is the way to go. If, for example, the user has a tab that is showing "Connecting..." with a spinner at the tab of tab switch, that animation will halt until the tab switch completes, which looks pretty bad and not smooth at all.

I think this is too big a hammer, and we need something more fine-grained.

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 32

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

Tab switching has forced painting and is thus working in quite different way these days.
And I don't really see extra paints locally.