1304363 - 2.23 - 4.78% tabpaint / tpaint (osx-10-10, windows7-32, windows8-64, windowsxp) regression on push 42a77283ee64 (Tue Sep 20 2016)

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

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

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

Regressions:

  3%  tpaint windowsxp opt               301.52 -> 311.26
  3%  tpaint osx-10-10 opt               358.24 -> 368.4
  2%  tpaint windows7-32 opt             300.04 -> 306.81
  2%  tpaint windows8-64 opt             286.52 -> 292.9


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=3325

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

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** 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)]

:dao, can you take a look at this regression.  After a bunch of retriggers it really points at this revision.  Let me know if there is work you think we should be able to do on this bug!

Comment 2

[Johann Hofmann [:johannh]]

I wonder if this is because of the added SVG styles or if there's a significant overhead that comes with consolidating SVGs into less files. Either way this is a bit disappointing, the new SVG files are in no way unreasonably large.

Comment 3

[Dão Gottwald [:dao]]

It might also be overhead from the CSS variables.

Comment 4

[Dão Gottwald [:dao]]

Diagnosis patch removing the CSS variables reverts the regressions:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b0ea142dbb5b&newProject=try&newRevision=2255af6f0b3e&framework=1

Comment 5

[Dão Gottwald [:dao]]

Attachment: Use preprocessing instead of CSS variables for identity block icon variants

Too bad that CSS variables can be such a drag.

I also moved the rules for identity-icons-brand.svg and connection-secure.svg into icons.inc.css to keep related rules together. This does mean that we'll needlessly duplicate these rules in browser.css and devedition.css.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=b0ea142dbb5b&newProject=try&newRevision=b65b05993cbb&framework=1&showOnlyImportant=0

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8794182 [details] [diff] [review]
Use preprocessing instead of CSS variables for identity block icon variants

Review of attachment 8794182 [details] [diff] [review]:
-----------------------------------------------------------------

So the level of pre-processing in this patch feels a bit much to me but I'm not sure I'm comfortable judging this yet.
If you don't mind I'll forward review to Gijs. Feel free to choose someone else if you like :)

Thanks!

::: browser/themes/shared/devedition.inc.css
@@ +191,5 @@
>    box-shadow: none !important;
>  }
>  
> +%filter substitution
> +%define selectorPrefix :root[devtoolstheme="dark"] 

I think there's a trailing space here

Comment 7

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #6)
> > +%define selectorPrefix :root[devtoolstheme="dark"] 
> 
> I think there's a trailing space here

It's intentionally there as part of the prefix.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8794182 [details] [diff] [review]
Use preprocessing instead of CSS variables for identity block icon variants

Review of attachment 8794182 [details] [diff] [review]:
-----------------------------------------------------------------

This is sad. I wish variables weren't such a perf drag. Is there a bug on file to ask platform to look at this?

::: browser/themes/shared/identity-block/icons.inc.css
@@ +23,5 @@
> +}
> +
> +@selectorPrefix@#urlbar[pageproxystate="valid"] > #identity-box.chromeUI > #identity-icon@selectorSuffix@ {
> +  list-style-image: url(chrome://branding/content/identity-icons-brand.svg);
> +}

So personally, I would put this and the connection-secure rules into identity-icons.inc.css directly after the includes. That way they're still together, and we avoid duplicating them everywhere, which seems suboptimal.

If you feel very strongly that's not a good idea, an alternative idea would be %ifdef'ing them so they only appear in one of the copies with some kind of define like "includeUnthemedIcons" or whatever that we only set for one of the includes.

Either of these would avoid issues where people expect rules added later in identity-icons.inc.css that change these images or their visibility to override these rules (e.g. if we want to hide the lock icon when forms submitting to non-secure origins are present), when they won't do that for devedition because they get repeated there.

@@ +56,5 @@
> +
> +@selectorPrefix@#urlbar[pageproxystate="valid"] > #identity-box.weakCipher > #connection-icon@selectorSuffix@,
> +@selectorPrefix@#urlbar[pageproxystate="valid"] > #identity-box.mixedDisplayContent > #connection-icon@selectorSuffix@,
> +@selectorPrefix@#urlbar[pageproxystate="valid"] > #identity-box.mixedDisplayContentLoadedActiveBlocked > #connection-icon@selectorSuffix@ {
> +  list-style-image: url(chrome://browser/skin/connection-mixed-passive-loaded.svg#icon@iconVariant@);

Separate bug (because this isn't changing), but the order of these rules does not make sense to me. There's 2 rules for passive loaded content surrounding the rule for active content, so right now a page with a weak cipher or mixed display content will show a passive mixed content warning even if there's also mixed active content. That seems wrong. Please check my logic, but I'm not missing something, can you confirm and/or file a followup bug?

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #8)
> Separate bug (because this isn't changing), but the order of these rules
> does not make sense to me. There's 2 rules for passive loaded content
> surrounding the rule for active content, so right now a page with a weak
> cipher or mixed display content will show a passive mixed content warning
> even if there's also mixed active content. That seems wrong. Please check my
> logic, but I'm not missing something, can you confirm and/or file a followup
> bug?

To clarify my thinking: I was originally going to ask to merge the two passive rules, and then realized that they should probably come before the active rule, but maybe it's intentional that in some cases we give signals for passive mixed content precedence over active mixed content? Either way, there should be a comment about why there's 2 rules if that's really intentional.

Comment 10

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #8)
> This is sad. I wish variables weren't such a perf drag. Is there a bug on
> file to ask platform to look at this?

Not sure. Not sure either if I could file a useful bug, as I have no clue what circumstances made the overhead particularly large here. Some kind of testcase would likely be useful if we want somebody to look into this.

> So personally, I would put this and the connection-secure rules into
> identity-icons.inc.css directly after the includes. That way they're still
> together, and we avoid duplicating them everywhere, which seems suboptimal.
> 
> If you feel very strongly that's not a good idea, an alternative idea would
> be %ifdef'ing them so they only appear in one of the copies with some kind
> of define like "includeUnthemedIcons" or whatever that we only set for one
> of the includes.
> 
> Either of these would avoid issues where people expect rules added later in
> identity-icons.inc.css that change these images or their visibility to
> override these rules (e.g. if we want to hide the lock icon when forms
> submitting to non-secure origins are present), when they won't do that for
> devedition because they get repeated there.

I prefer keeping this as is. The additions to the selectors affect their specificity which could mix things up if we only repeat some of the rules.

> Separate bug (because this isn't changing), but the order of these rules
> does not make sense to me. There's 2 rules for passive loaded content
> surrounding the rule for active content, so right now a page with a weak
> cipher or mixed display content will show a passive mixed content warning
> even if there's also mixed active content. That seems wrong. Please check my
> logic, but I'm not missing something, can you confirm and/or file a followup
> bug?

By now you've already thought more about this than I have. Can you please file that bug?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2596f38f83f
Use preprocessing instead of CSS variables for identity block icon variants. r=gijs

Comment 12

[Avi Halachmi (:avih)]

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> > This is sad. I wish variables weren't such a perf drag. Is there a bug on
> > file to ask platform to look at this?
> 
> Not sure. Not sure either if I could file a useful bug, as I have no clue
> what circumstances made the overhead particularly large here. Some kind of
> testcase would likely be useful if we want somebody to look into this.

While it might be hard to file a useful bug here, I think the subject is worth discussing in general and it would be nice to have some understanding of how to act on such cases.

It's not the first time and probably not the last either that front end changes something which ends up slower due to how stuff is implemented outside of front-end people control.

As far as I recall, in most cases there was nothing wrong per-se with the front end usage of things, but it happens to perform with noticeable difference.

I think it would be good to have some high level guidelines for such cases.

Comment 13

[:Gijs (he/him)]

Filed bug 1305676 for the identity-block icons that are confusing.

I'll keep needinfo to look at having a testcase for the variables stuff.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d031de38165
followup, fixing typo

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c2596f38f83f
https://hg.mozilla.org/mozilla-central/rev/4d031de38165

Comment 16

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

thanks for fixing this- I have verified on the graphs that all the regressions are fixed here.

Comment 18

[Alison Shiue]

Talos has detected a tpaint windowsxp opt e10s regression on push c2596f38f83f62502324f8d1ae3963b7db7956aa. 

Regressions:

  4%  tpaint windowsxp opt e10s     289.61 -> 300.67

Here is the zooming better view: https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-inbound,0bda8c20dbca1e0d32dcc952a4473e73ce7d4352,1,1%5D&zoom=1474966363351.8428,1474974528776.1497,285.0262052500023,305.5604415071994&selected=%5Bmozilla-inbound,0bda8c20dbca1e0d32dcc952a4473e73ce7d4352,37080,36680872,1%5D

I also did some retriggers on win7 and win8 to check if tpaint opt e10s regression happened on all windows platforms, but there is nothing.
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=cb1127ffac277fc5cbf78dbae2abf96caaad99f0&newProject=mozilla-inbound&newRevision=c2596f38f83f62502324f8d1ae3963b7db7956aa&framework=1&showOnlyImportant=0

Comment 19

[Alison Shiue]

Here is the detected alert: https://treeherder.mozilla.org/perf.html#/alerts?id=3457

Comment 20

[Dão Gottwald [:dao]]

(In reply to Alison Shiue from comment #18)
> Talos has detected a tpaint windowsxp opt e10s regression on push
> c2596f38f83f62502324f8d1ae3963b7db7956aa. 

Lol. This makes no sense. If it's only WinXP, I suggest we ignore this.

Comment 21

[Dão Gottwald [:dao]]

Uplifted as part of bug 1303291:
https://hg.mozilla.org/releases/mozilla-aurora/rev/805d5fc3d66c

Comment 22

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #10)
> (In reply to :Gijs Kruitbosch from comment #8)
> > This is sad. I wish variables weren't such a perf drag. Is there a bug on
> > file to ask platform to look at this?
> 
> Not sure. Not sure either if I could file a useful bug, as I have no clue
> what circumstances made the overhead particularly large here. Some kind of
> testcase would likely be useful if we want somebody to look into this.

The more I think about this, the more I feel the same. I mean, we have an obvious testcase of a trypush including backing out the patches in this bug, but I don't know that I'd be able to whittle that down to something more sane very easily.

Dan, can you suggest how we might go about getting you / someone else in Layout a better testcase about the impact CSS variables are having here? (so that hopefully we can improve perf both for our sake and the web! :-) )

Comment 23

[Daniel Holbert [:dholbert]]

redirecting needinfo to heycam, who's worked more on & knows more about CSS variables than I.

Comment 24

[Cameron McCormack (:heycam)]

We have bug 1199054 which captures one aspect of CSS Variables performance.  My feeling recently has been to avoid putting effort into improving our CSS Variables performance, since some time next year the stylo work will make it moot.