Closed Bug 1451460 Opened 3 years ago Closed 3 years ago

tps test shouldn't use MozAfterPaint to measure tab to completing tab switches

Categories

(Testing :: Talos, defect)

Version 3
defect
Not set
normal

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)

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.
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
Blocks: 1445214
Blocks: 1451522
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.
(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)
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)
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!)
(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)
(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 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+
(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 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 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 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 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 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+
Whoops - forgot to update the patches with the schema.json. Here's the interdiff: 

https://reviewboard.mozilla.org/r/234178/
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+
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. :)
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
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.