1323319 - Force painting of out-of-process tab switches should work for first activation

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

In bug 1279086, billm added the capability to force the content process to interrupt JS asap and force the painting of a tab we've switched to.

Due to a tabpaint talos regression[1], a condition was added in [1] which we skip forcing paint for the initial activation of a tab.

I suggest we try to do that force paint, even for first activation. I think I know a way we can alter the talos test to avoid the regression.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1279086#c35
[2]: http://searchfox.org/mozilla-central/rev/594937fec2e2fc45fa9308ba2fb964816631f017/dom/ipc/TabParent.cpp#2676

Comment 1

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

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

Comment 2

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

Attachment: Bug 1323319 - Make sure we can force paint remote tabs even during the first activation.

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

Comment 3

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

I did some try pushes:

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

Force Paint Early Patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93d6c66af1ac85dadfed1cde4afd12a6bde716e2

Comparison: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5b1b0ef062cb92d40d12be45438a83e8f9983c9a&newProject=try&newRevision=93d6c66af1ac85dadfed1cde4afd12a6bde716e2&framework=1&showOnlyImportant=0

OS X numbers are still coming in, but Windows and Linux seem to be in agreement that there's no regression here on the "other" talos tests (which included tabpaint, the test we were originally worried about).

Comment 4

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

Comment on attachment 8820882 [details]
Bug 1323319 - Make sure we can force paint remote tabs even during the first activation.

https://reviewboard.mozilla.org/r/100270/#review101944

The patch itself looks fine. Could you also check TART and TPS? The original regression report was bug 1297024, which included regressions in a number of tests.

Comment 5

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

Sure. TART looks good. With TPS, I see a small regression on OS X and Linux:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5b1b0ef062cb92d40d12be45438a83e8f9983c9a&newProject=try&newRevision=93d6c66af1ac85dadfed1cde4afd12a6bde716e2&framework=1

However, as gabor found out for e10s-multi, TPS doesn't do a pass, switching through the tp5 tabs to warm up caches, _before_ doing measurement. I believe his plan is to modify TPS to do a warm-up. I'd like to see if this regression goes away with that change.

I'll see if I can do a run with that change next week when I'm back. (PTO'ing poorly, but I'm also on a bus)

Comment 6

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

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

Comment 7

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

In bug 1317312, gabor has patches (which I just r+'d) that cause our Talos tests to measure the right things for e10s-multi. This has the added bonus of getting rid of the first-activation force paint in the measurement, which we probably didn't care about either (since it probably just meant that we painted too fast and the test's JS was too late to attach and hear the MozAfterPaint event).

With those patches applied, the regression I found in comment 5 (which, after retriggers, ended up being a "high confidence" regression on OS X), disappeared: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=46d21c218f42&newProject=try&newRevision=7baee81ae3d0&framework=1&showOnlyImportant=0

So I'm going to mark this depending on the patches in bug 1317312, and land this once that stuff is in tree.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e96dcbf938f37d1537b66c480ccd8ff32fd9262
Bug 1323319 - Make sure we can force paint remote tabs even during the first activation. r=billm

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e96dcbf938f

Comment 10

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

Unsurprisingly, this patch has definitely had an impact on our tab spinners on Nightly.

Looking at the graph over the last few days, there's nice fall-off once we hit the build where this patch landed. This has knocked a few percentage points off of the "% of e10s users who have seen at least one spinner" graph:

http://imgur.com/XaMCb3Z

Source: https://mikeconley.github.io/bug1310250/

Comment 11

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

Comment on attachment 8820882 [details]
Bug 1323319 - Make sure we can force paint remote tabs even during the first activation.

Approval Request Comment
[Feature/Bug causing the regression]:

e10s

[User impact if declined]:

Users have a higher probability of seeing tab switch spinners when a tab is selected for the first time.

[Is this code covered by automated tests?]:

Tab switching certainly is, yes.

[Has the fix been verified in Nightly?]:

Yes, and we've verified that this has improved our tab switching behaviour via Telemetry.

[Needs manual test from QE? If yes, steps to reproduce]: 

No.

[List of other uplifts needed for the feature/fix]:

We'll need the following patches uplifted as well:
* Part 2 from bug 1317312
* Bug 1285176 (since this patch exacerbates that intermittent)

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

This patch makes us stop skipping force paint on the first selection of a tab. The force painting stuff has been working well for months, and is about to ship to release in a few days. This patch makes us use it more often.

[String changes made/needed]:

None.

Comment 12

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

Note that bug 1285176 has already uplifted to aurora.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8820882 [details]
Bug 1323319 - Make sure we can force paint remote tabs even during the first activation.

force paint on first activation, aurora52+

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2eab2dda495a