Mike Conley (:mconley) (:⚙️) Assignee
	Description • 7 years ago

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.

	Mike Conley (:mconley) (:⚙️) Assignee
	Comment 1 • 7 years ago

I noticed this in bug 1447193 comment 55.

	Ionuț Goldan [:igoldan]
	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

	Mike Conley (:mconley) (:⚙️) 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.

	Mike Conley (:mconley) (:⚙️) 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

	Mike Conley (:mconley) (:⚙️) 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!)

	Robert Wood [:rwood]
	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

	Kris Maglione [:kmag]
	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).

	Robert Wood [:rwood]
	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

	Robert Wood [:rwood]
	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

	Robert Wood [:rwood]
	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

	Robert Wood [:rwood]
	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

	Robert Wood [:rwood]
	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!

	Robert Wood [:rwood]
	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

	Robert Wood [:rwood]
	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'.

	Mike Conley (:mconley) (:⚙️) 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/

	Robert Wood [:rwood]
	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!

	Mike Conley (:mconley) (:⚙️) 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. :)

	Pulsebot
	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

	Natalia Csoregi [:nataliaCs]
	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

	Joel Maher ( :jmaher ) (UTC -8)
	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