Closed
Bug 1451460
Opened 7 years ago
Closed 7 years ago
tps test shouldn't use MozAfterPaint to measure tab to completing tab switches
Categories
(Testing :: Talos, defect)
Tracking
(firefox-esr52 unaffected, firefox59 unaffected, firefox60 unaffected, firefox61+ fixed)
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | --- | unaffected |
firefox61 | + | fixed |
People
(Reporter: mconley, Assigned: mconley)
References
Details
Attachments
(7 files)
(deleted),
text/x-review-board-request
|
rwood
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rwood
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rwood
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rwood
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rwood
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rwood
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
rwood
:
review+
|
Details |
The tps test uses a MozAfterPaint listener in the content process to know when layers have been uploaded to the compositor. One thing that seems to be sensitive to is paints that occur in quick succession. It seems if we do two paints in quick succession, we only get one MozAfterPaint event even though two uploads occurred. Here's an example in a profile: https://perfht.ml/2GRzN4a The tps test should probably monitor for MozLayerTreeReady in the parent process instead of waiting for MozAfterPaint in the content process.
Comment 2•7 years ago
|
||
The false tps regressions showed up: == Change summary for alert #12519 (as of Wed, 04 Apr 2018 18:27:28 GMT) == Regressions: 21% tps linux64 opt e10s stylo 18.20 -> 21.98 8% tps windows10-64 pgo e10s stylo18.55 -> 20.11 8% tps windows7-32 pgo e10s stylo 19.56 -> 21.18 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12519
Updated•7 years ago
|
Assignee: nobody → mconley
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox61:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox61:
--- → +
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (obsolete) |
Assignee | ||
Comment 11•7 years ago
|
||
Bah, forgot to include schema.json. Obsoleting old comment. I think I've got something that works here. I'm pleased to note that converting TPS to a WebExtension was a lot simpler than I thought it'd be. The WebExtension experiment API makes it pretty straight-forward, imo. Doing some comparisons on try: Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d90818ff1705cf8ab37ede0062f7bba1d7f84fea TPS WebExtension refactor: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d861c997ef28142915422634138d28d1a09249a0 TPS WebExtension refactor with bug 1447193 backed out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b92349936809e3e8c512c1b9a6b5acfb52c7c0e2 We should hopefully see no significant difference between the TPS refactor pushes - that'll verify my strong suspicion that the regression noted in comment 2 is more of a test issue than a real performance problem.
Assignee | ||
Comment 12•7 years ago
|
||
(In reply to Mike Conley (:mconley) (:βοΈ) (Totally backlogged on reviews and needinfos) from comment #11) > We should hopefully see no significant difference between the TPS refactor > pushes - that'll verify my strong suspicion that the regression noted in > comment 2 is more of a test issue than a real performance problem. Suspicion confirmed: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=d861c997ef28&newProject=try&newRevision=b92349936809&framework=1&showOnlyImportant=1
Flags: needinfo?(mconley)
Assignee | ||
Updated•7 years ago
|
Attachment #8965438 -
Flags: review?(rwood)
Attachment #8965439 -
Flags: review?(rwood)
Attachment #8965440 -
Flags: review?(rwood)
Attachment #8965441 -
Flags: review?(rwood)
Attachment #8965442 -
Flags: review?(rwood)
Attachment #8965443 -
Flags: review?(rwood)
Attachment #8965444 -
Flags: review?(rwood)
Assignee | ||
Comment 13•7 years ago
|
||
Incidentally, I also filed bug 1451860 to rename tps to tabswitch, since there's already a TPS test (too many people trying to make an Office Space joke!)
Comment 14•7 years ago
|
||
(In reply to Mike Conley (:mconley) (:βοΈ) (Totally backlogged on reviews and needinfos) from comment #11) > Bah, forgot to include schema.json. Obsoleting old comment. > > I think I've got something that works here. I'm pleased to note that > converting TPS to a WebExtension was a lot simpler than I thought it'd be. > The WebExtension experiment API makes it pretty straight-forward, imo. Hi Mike, I'm looking at your patches now, reading about WebExtension experiments - quick question, will this work with beta (as we run talos there too). The WebExtension experiments documentation [1] indicates that "Currently experiments can only be loaded in Firefox Nightly" but hopefully that's out of date (?) [1] https://webextensions-experiments.readthedocs.io/en/latest/basics.html
Flags: needinfo?(mconley)
Comment 15•7 years ago
|
||
(In reply to Robert Wood [:rwood] from comment #14) > Hi Mike, I'm looking at your patches now, reading about WebExtension > experiments - quick question, will this work with beta (as we run talos > there too). The WebExtension experiments documentation [1] indicates that > "Currently experiments can only be loaded in Firefox Nightly" but hopefully > that's out of date (?) They have essentially the same restrictions as legacy extensions. They can be loaded in beta if they're specially signed, or loaded temporarily (e.g., via Marionette). So, migrating from a bootstrapped extension to an embedded experiment doesn't really change the situation significantly (except to the extent that bootstrapped extensions are going away).
Flags: needinfo?(mconley)
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8965438 [details] Bug 1451460 - Stub out WebExtension bits for tps. https://reviewboard.mozilla.org/r/234180/#review240700 LGTM
Attachment #8965438 -
Flags: review?(rwood) → review+
Comment 17•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #15) > (In reply to Robert Wood [:rwood] from comment #14) > > > Hi Mike, I'm looking at your patches now, reading about WebExtension > > experiments - quick question, will this work with beta (as we run talos > > there too). The WebExtension experiments documentation [1] indicates that > > "Currently experiments can only be loaded in Firefox Nightly" but hopefully > > that's out of date (?) > > They have essentially the same restrictions as legacy extensions. They can > be loaded in beta if they're specially signed, or loaded temporarily (e.g., > via Marionette). So, migrating from a bootstrapped extension to an embedded > experiment doesn't really change the situation significantly (except to the > extent that bootstrapped extensions are going away). Thanks Kris
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8965439 [details] Bug 1451460 - Trim some dead code from the TPS test. https://reviewboard.mozilla.org/r/234182/#review240714 LGTM
Attachment #8965439 -
Flags: review?(rwood) → review+
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8965440 [details] Bug 1451460 - Stop using Task.jsm in TPS test. https://reviewboard.mozilla.org/r/234184/#review240718
Attachment #8965440 -
Flags: review?(rwood) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8965441 [details] Bug 1451460 - Get rid of single-process support in TPS talos test. https://reviewboard.mozilla.org/r/234186/#review240720 Awesome, more single-process code is history!
Attachment #8965441 -
Flags: review?(rwood) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8965442 [details] Bug 1451460 - Set opacity to 0 on the tab strip instead of visibility: hidden to hide separators too. https://reviewboard.mozilla.org/r/234188/#review240726 Cool workaround
Attachment #8965442 -
Flags: review?(rwood) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8965443 [details] Bug 1451460 - Switch to using MozLayerTreeReady in the parent process to know when to stop the TPS stopwatch. https://reviewboard.mozilla.org/r/234190/#review240738 Nice! ::: testing/talos/talos/tests/tabswitch/api.js:160 (Diff revision 1) > let browser = tab.linkedBrowser; > let gBrowser = tab.ownerGlobal.gBrowser; > - let window = tab.ownerGlobal; > > await loadTPSContentScript(browser); > - let start = Math.floor(window.performance.timing.navigationStart + window.performance.now()); > + let start = Cu.now(); Does this map to window.performance.now? Just curious ::: testing/talos/talos/tests/tabswitch/api.js:214 (Diff revision 1) > return new Promise((resolve) => { > - let mm = browser.messageManager; > - mm.addMessageListener("TPS:ContentSawPaint", function onContentPaint(msg) { > - mm.removeMessageListener("TPS:ContentSawPaint", onContentPaint); > - resolve(msg.data.time); > - }); > + browser.addEventListener("MozLayerTreeReady", function onLayersReady(event) { > + let now = Cu.now(); > + TalosParentProfiler.mark("MozLayerTreeReady seen by tps"); > + resolve(now); > + }, { once: true }); That's really cool re: 'once'.
Attachment #8965443 -
Flags: review?(rwood) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
Whoops - forgot to update the patches with the schema.json. Here's the interdiff: https://reviewboard.mozilla.org/r/234178/
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8965444 [details] Bug 1451460 - Get rid of more dead code from the TPS Talos test. https://reviewboard.mozilla.org/r/234192/#review240742 Last patch LGTM. Ok, I also applied each patch locally (included the latest update) and ran the talos tps test (on OSX) via ./mach talos-test --activeTests tps. The test worked great. Looks like some intermittents on your try push but it's the task timeout exceeding which have existed before this - so we may need to extend the task timeout for g2 (outside of this bug). I'd say land these puppies! :) This is really cool thanks Mike!
Attachment #8965444 -
Flags: review?(rwood) → review+
Assignee | ||
Comment 32•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8965443 [details] Bug 1451460 - Switch to using MozLayerTreeReady in the parent process to know when to stop the TPS stopwatch. https://reviewboard.mozilla.org/r/234190/#review240738 > Does this map to window.performance.now? Just curious Only sorta, in that they both are related to time. `window.performance.now()` returns [the amount of time since the document in the window started loading](https://developer.mozilla.org/en-US/docs/Web/API/Performance/now). `Cu.now()` is a [privileged-code-only API that returns the amount of time since process start-up](https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/js/xpconnect/idl/xpccomponents.idl#664-668). I used `Cu.now()` mostly as a simplification. > That's really cool re: 'once'. Yeah, it's super handy. :)
Comment 33•7 years ago
|
||
Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1974764ea946 Stub out WebExtension bits for tps. r=rwood https://hg.mozilla.org/integration/autoland/rev/7dd5cc7c827a Trim some dead code from the TPS test. r=rwood https://hg.mozilla.org/integration/autoland/rev/6a084f6f8a77 Stop using Task.jsm in TPS test. r=rwood https://hg.mozilla.org/integration/autoland/rev/1d8809f88dba Get rid of single-process support in TPS talos test. r=rwood https://hg.mozilla.org/integration/autoland/rev/4a73ac997f54 Set opacity to 0 on the tab strip instead of visibility: hidden to hide separators too. r=rwood https://hg.mozilla.org/integration/autoland/rev/c40e37a06d76 Switch to using MozLayerTreeReady in the parent process to know when to stop the TPS stopwatch. r=rwood https://hg.mozilla.org/integration/autoland/rev/c97f80825156 Get rid of more dead code from the TPS Talos test. r=rwood
Comment 34•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1974764ea946 https://hg.mozilla.org/mozilla-central/rev/7dd5cc7c827a https://hg.mozilla.org/mozilla-central/rev/6a084f6f8a77 https://hg.mozilla.org/mozilla-central/rev/1d8809f88dba https://hg.mozilla.org/mozilla-central/rev/4a73ac997f54 https://hg.mozilla.org/mozilla-central/rev/c40e37a06d76 https://hg.mozilla.org/mozilla-central/rev/c97f80825156
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 35•7 years ago
|
||
some perf improvements from this change: == Change summary for alert #12614 (as of Tue, 10 Apr 2018 00:57:55 GMT) == Improvements: 40% tps linux64 opt e10s stylo 22.08 -> 13.21 35% tps windows7-32 pgo e10s stylo 21.03 -> 13.71 35% tps windows10-64 pgo e10s stylo20.38 -> 13.31 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=12614
You need to log in
before you can comment on or make changes to this bug.
Description
•