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

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

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

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 1

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

I noticed this in bug 1447193 comment 55.

Comment 2

[Ionuț Goldan [:igoldan]]

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

Comment 3

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

Attachment: Bug 1451460 - Stub out WebExtension bits for tps.

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

Comment 4

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

Attachment: Bug 1451460 - Trim some dead code from the TPS test.

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

Comment 5

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

Attachment: Bug 1451460 - Stop using Task.jsm in TPS test.

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

Comment 6

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

Attachment: Bug 1451460 - Get rid of single-process support in TPS talos test.

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

Comment 7

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

Attachment: Bug 1451460 - Set opacity to 0 on the tab strip instead of visibility: hidden to hide separators too.

This Talos test is more concerned with how much it costs to switch tabs in the simple case
where the tab strip isn't scrolling around. To avoid having that noise introduced, the tab
strip is hidden by default. Unfortunately, with the way that we style the tabs in Photon,
some parts of the tabs are still visible and reintroduces the noise. Setting opacity to
0 hides it fully.

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

Comment 8

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

Attachment: Bug 1451460 - Switch to using MozLayerTreeReady in the parent process to know when to stop the TPS stopwatch.

MozLayerTreeReady is fired once the parent process hears from the compositor that the layers
from a remote tab have been painted and uploaded. For tab switching, this is a far more accurate
assessment of when the tab is actually presented to the user, since this is the event that the
async tab switcher also listens for.

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

Comment 9

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

Attachment: Bug 1451460 - Get rid of more dead code from the TPS Talos test.

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

Comment 10

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

Okay, 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=7aa417703cd0ec6f1eb365804c5381dd018d7e6e
TPS WebExtension refactor with bug 1447193 backed out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed76d7f81563db3b05911f10ac120b384d01c76f

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.

Comment 11

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

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.

Comment 12

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

(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

Comment 13

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

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

[Robert Wood [:rwood]]

(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

Comment 15

[Kris Maglione [:kmag]]

(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).

Comment 16

[Robert Wood [:rwood]]

Comment on attachment 8965438 [details]
Bug 1451460 - Stub out WebExtension bits for tps.

https://reviewboard.mozilla.org/r/234180/#review240700

LGTM

Comment 17

[Robert Wood [:rwood]]

(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

[Robert Wood [:rwood]]

Comment on attachment 8965439 [details]
Bug 1451460 - Trim some dead code from the TPS test.

https://reviewboard.mozilla.org/r/234182/#review240714

LGTM

Comment 19

[Robert Wood [:rwood]]

Comment on attachment 8965440 [details]
Bug 1451460 - Stop using Task.jsm in TPS test.

https://reviewboard.mozilla.org/r/234184/#review240718

Comment 20

[Robert Wood [:rwood]]

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!

Comment 21

[Robert Wood [:rwood]]

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

Comment 22

[Robert Wood [:rwood]]

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'.

Comment 23

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

Comment on attachment 8965438 [details]
Bug 1451460 - Stub out WebExtension bits for tps.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234180/diff/1-2/

Comment 24

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

Comment on attachment 8965439 [details]
Bug 1451460 - Trim some dead code from the TPS test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234182/diff/1-2/

Comment 25

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

Comment on attachment 8965440 [details]
Bug 1451460 - Stop using Task.jsm in TPS test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234184/diff/1-2/

Comment 26

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

Comment on attachment 8965441 [details]
Bug 1451460 - Get rid of single-process support in TPS talos test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234186/diff/1-2/

Comment 27

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

Comment on attachment 8965442 [details]
Bug 1451460 - Set opacity to 0 on the tab strip instead of visibility: hidden to hide separators too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234188/diff/1-2/

Comment 28

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

Comment on attachment 8965443 [details]
Bug 1451460 - Switch to using MozLayerTreeReady in the parent process to know when to stop the TPS stopwatch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234190/diff/1-2/

Comment 29

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

Comment on attachment 8965444 [details]
Bug 1451460 - Get rid of more dead code from the TPS Talos test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234192/diff/1-2/

Comment 30

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

Whoops - forgot to update the patches with the schema.json. Here's the interdiff: 

https://reviewboard.mozilla.org/r/234178/

Comment 31

[Robert Wood [:rwood]]

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!

Comment 32

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

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

[Pulsebot]

Pushed by [e-mail]:
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

[Natalia Csoregi [:nataliaCs]]

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

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

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