Closed Bug 1323319 Opened 5 years ago Closed 5 years ago
Force painting of out-of-process tab switches should work for first activation
59 bytes, text/x-review-board-request
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, a condition was added in  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. : https://bugzilla.mozilla.org/show_bug.cgi?id=1279086#c35 : http://searchfox.org/mozilla-central/rev/594937fec2e2fc45fa9308ba2fb964816631f017/dom/ipc/TabParent.cpp#2676
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 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)
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
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
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 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+
You need to log in before you can comment on or make changes to this bug.