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

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

50 Branch
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bda890eef136
Comment hidden (mozreview-request)
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

4 months ago
mozreview-review
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.
Attachment #8820882 - Flags: review?(wmccloskey) → review+
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)
Flags: needinfo?(mconley)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d2a19481e48
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.
Depends on: 1317312
Flags: needinfo?(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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e96dcbf938f
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee: nobody → 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/
See Also: → bug 1285176
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.
Attachment #8820882 - Flags: approval-mozilla-aurora?
Note that bug 1285176 has already uplifted to aurora.
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+
Attachment #8820882 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 14

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/2eab2dda495a
status-firefox52: --- → fixed
You need to log in before you can comment on or make changes to this bug.