1280951 - 2.29 - 4.46% tp5o XRes (linux64) regression on push eebf02e5a399 (Fri Jun 10 2016)

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

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

Talos has detected a Firefox performance regression from push eebf02e5a399. As author of one of the patches included in that push, we need your help to address this regression.

This is a list of all known regressions and improvements related to the push:
https://treeherder.mozilla.org/perf.html#/alerts?id=1515

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tp5

Reproducing and debugging the regression:
If you would like to re-run this Talos test on a potential fix, use try with the following syntax:
try: -b o -p linux64 -u none -t tp5o,tp5o-e10s --rebuild 5  # add "mozharness: --spsProfile" to generate profile data

(we suggest --rebuild 5 to be more confident in the results)

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.lorg/Buildbot/Talos/Running#Running_locally_-_Source_Code

Then run the following command from the directory where you set up Talos:
talos --develop -e [path]/firefox -a tp5o

(add --e10s to run tests in e10s mode)

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations:
https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling

Comment 1

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

the graph for this test needs some zooming love, but here is a better view:
https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bfx-team,2440041b70bf8c7aca5f2e0acc169916f4374358,1,1%5D&series=%5Bmozilla-inbound,2440041b70bf8c7aca5f2e0acc169916f4374358,1,1%5D&series=%5Bmozilla-inbound,36195bda5c567aa358f855f4667c7ae38744d194,1,1%5D&zoom=1465479310226.228,1465581309328.3904,3244.5875335193596,4572.604756541332

I did some retriggers, this seems like the root cause- there are 4 patches though, I am not sure which is the offender:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=eebf02e5a399

Possibly this is something we need to live with, it would be good to understand this prior to wontfix or spending time fixing this.

:karlt, can you take a look at this and figure out why our XRes measurements are increasing?

Comment 2

[Karl Tomlinson (:karlt)]

Thanks, Joel.
It is somewhat surprising that there is a difference as significant as this.
I'll look into it.

Comment 3

[Karl Tomlinson (:karlt)]

This was triggered by https://hg.mozilla.org/integration/mozilla-inbound/rev/80425dc038ac
probably because it triggered instantiation of the tooltip widget.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: bug 1280951 copy tooltip style context and cache that instead of widget

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

Comment 5

[Karl Tomlinson (:karlt)]

The patch returns tp5o_xres from 3928.19 to 3832.24
https://treeherder.mozilla.org/#/jobs?repo=try&revision=874e8b04f42e&selectedJob=22627377
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfdc725935a9&selectedJob=22627521

(The treeherder display is confusing because the former shows
https://hg.mozilla.org/try/rev/e8705ee011170c87ff5c01042d441514150a91f0
even though it is not an ancestor.)

Comment 6

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

wow, thanks for figuring this out so fast!

Comment 7

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

I did some retriggers on the tp job and we can view the delta between both patches here:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=dfdc725935a9&newProject=try&newRevision=874e8b04f42e&framework=1&showOnlyImportant=0

Comment 8

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

I don't think the new patch is helping, after some retriggers it looks like we have fairly consisten 3928 vs 3832 numbers- maybe I have the try pushes mixed up and we have a 2.5% win vs a loss!

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to Joel Maher (:jmaher) from comment #8)
> I don't think the new patch is helping, after some retriggers it looks like
> we have fairly consisten 3928 vs 3832 numbers- maybe I have the try pushes
> mixed up and we have a 2.5% win vs a loss!

(In reply to Joel Maher (:jmaher) from comment #7)
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=dfdc725935a9&newProject=try&newRevision=874e8b04f42e&framework=1&showOnlyImportant=0

(In reply to Karl Tomlinson (ni?:karlt) from comment #5)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=874e8b04f42e&selectedJob=22627377
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=dfdc725935a9&selectedJob=22627521
> 
> (The treeherder display is confusing because the former shows
> https://hg.mozilla.org/try/rev/e8705ee011170c87ff5c01042d441514150a91f0
> even though it is not an ancestor.)

874e8b04f42e (so-called newRevision/new) is the reference, without e8705ee01117 as an ancestor.  e8705ee01117 is displayed on treeherder even though it is another head.
The parent of 874e8b04f42e is on m-c.

dfdc725935a9 (so-called originalRevision/base) has the fix e8705ee01117 as the parent, which is parented on the same m-c revision as 874e8b04f42e.
Treeherder does not show e8705ee01117 because it was not new in this push.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

The patch looks fine but I don't have pre 3.20 gtk handy right now so I'm going to test it there this afternoon.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8763800 [details]
bug 1280951 copy tooltip style context and cache that instead of widget

https://reviewboard.mozilla.org/r/59916/#review56856

I tested the pre3.20 and looks fine. Just need to address the 3.20 cache problem.

::: widget/gtk/WidgetStyleCache.cpp:210
(Diff revision 1)
>  
>  /* GetCssNodeStyleInternal is used by Gtk >= 3.20 */
>  static GtkStyleContext*
>  GetCssNodeStyleInternal(WidgetNodeType aNodeType)
>  {
> -  GtkStyleContext* style = sStyleStorage[aNodeType];
> +  MOZ_ASSERT(!sStyleStorage[aNodeType]);

We need the style cache here or at CreateChildCSSNode().

When GetCssNodeStyleInternal() is called recusrively (for instance MOZ_GTK_SCROLLBAR_TROUGH_HORIZONTAL -> MOZ_GTK_SCROLLBAR_CONTENTS_HORIZONTAL) it asserts here and creates the style instead of the cached one.

like:
 static GtkStyleContext*
 CreateChildCSSNode(const char* aName, WidgetNodeType aParentNodeType)
 {
-  return CreateCSSNode(aName, GetCssNodeStyleInternal(aParentNodeType));
+  GtkStyleContext* styleParent = sStyleStorage[aParentNodeType];
+  if (!styleParent)
+    styleParent = GetCssNodeStyleInternal(aParentNodeType);
+
+  return CreateCSSNode(aName, styleParent); 
 }

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8763800 [details]
bug 1280951 copy tooltip style context and cache that instead of widget

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

Comment 13

[Karl Tomlinson (:karlt)]

https://reviewboard.mozilla.org/r/59916/#review56856

> We need the style cache here or at CreateChildCSSNode().
> 
> When GetCssNodeStyleInternal() is called recusrively (for instance MOZ_GTK_SCROLLBAR_TROUGH_HORIZONTAL -> MOZ_GTK_SCROLLBAR_CONTENTS_HORIZONTAL) it asserts here and creates the style instead of the cached one.
> 
> like:
>  static GtkStyleContext*
>  CreateChildCSSNode(const char* aName, WidgetNodeType aParentNodeType)
>  {
> -  return CreateCSSNode(aName, GetCssNodeStyleInternal(aParentNodeType));
> +  GtkStyleContext* styleParent = sStyleStorage[aParentNodeType];
> +  if (!styleParent)
> +    styleParent = GetCssNodeStyleInternal(aParentNodeType);
> +
> +  return CreateCSSNode(aName, styleParent); 
>  }

Ah, yes.  Thanks.  I've reverted this part, and instead checking sStyleStorage just for MOZ_GTK_TOOLTIP in GetWidgetStyleInternal() for now.

Comment 14

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0a94d2f83f0

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8763800 [details]
bug 1280951 copy tooltip style context and cache that instead of widget

https://reviewboard.mozilla.org/r/59916/#review57028

Looks fine, Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d56d7039aee0
copy tooltip style context and cache that instead of widget r=stransky+263117

Comment 17

[Robert Wood [:rwood]]

Excellent, this patch has triggered an improvement alert on Talos.

https://treeherder.mozilla.org/perf.html#/alerts?id=1574

So far it has shown a 4.3% improvement in tp5o XRes linux64 opt e10s. I retriggered several times and the results are consistent.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d56d7039aee0

Comment 19

[Alison Shiue]

As a note, this patch d56d7039aee0 has been merged into Aurora, here is the perf alert from Aurora:
https://treeherder.mozilla.org/perf.html#/alerts?id=2185

  tp5o XRes linux64 pgo e10s - 95.45% worse

Here is the zooming better view:
https://treeherder.mozilla.org/perf.html#/graphs?timerange=5184000&series=%5Bmozilla-aurora,20c5b14935f9decd1d00eb2c266a53ea8cf004af,1,1%5D&series=%5Bmozilla-inbound,20c5b14935f9decd1d00eb2c266a53ea8cf004af,1,1%5D&zoom=1466461330936.1177,1470189206000,4471.959182024444,6204.843365058914

Comment 20

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

I think this issue is more of a bi-modal test (most likely the fix to this bug caused the bi-modal distribution) and we see issues where some runs can show all data points on the high or low mode.