1273628 - 2.09 - 5.37% sessionrestore / tpaint (windows7-32, windows8-64, windowsxp) regression on push 9ffc0498ad7e (Tue May 17 2016)

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

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

Talos has detected a Firefox performance regression from push 9ffc0498ad7e. 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=1227

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#tpaint
https://wiki.mozilla.org/Buildbot/Talos/Tests#sessionrestore.2Fsessionrestore_no_auto_restore

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 win64,win32 -u none -t other[Windows 8,Windows 7,Windows XP] --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 tpaint:sessionrestore

(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 by Friday, 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)]

I did some retriggers on the 'o' job and found this to be the root cause:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffc0498ad7efb3a748b8a46be8cfa56002ec40b

here is a compare view:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=7fe18230f273&newProject=mozilla-inbound&newRevision=9ffc0498ad7e&framework=1&showOnlyImportant=0

^ note: more data is coming in, right now we have these regressions verified:
4-5% tpaint win xp/7/8
2% sessionrestore win 7

:baku, can you take a look at this and help us figure out how to resolve this bug?

Comment 2

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

This is almost certainly either the initialization of UserContextStyleManager in delayedStartup, or the introduction of the new stylesheet. We should probably confirm that theory, and then try to figure out how to mitigate.

Comment 3

[Matthew N. [:MattN]]

It could just be the usage of CSS variables like we saw in bug 1122726 comment 7. IIRC heycam had some idea to improve variable performance but it wasn't a high priority for him. I don't know the specifics of that fix or see a platform bug filed but I believe the needinfo on that bug is about that.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 49 since this is a recent regression.

Comment 5

[Andrea Marchesini [:baku]]

It seem the bug is more generic than my patch. We have performance issues with CSS variables. I can easily propose a change so that my code runs only if containers feature is enabled, but at some point, this will be the default, and this regression will appear again.

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

jet, is this something you want to weigh in on ?  How much of a performance regression is acceptable here?

Comment 7

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

(In reply to Andrea Marchesini (:baku) from comment #5)
> It seem the bug is more generic than my patch. We have performance issues
> with CSS variables. I can easily propose a change so that my code runs only
> if containers feature is enabled, but at some point, this will be the
> default, and this regression will appear again.

Yeah, we might want to disable this if containers are disabled - perhaps that'll buy us enough time to optimize the bottleneck here.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

It would be better to resolve this before the merge on Monday so that we don't end up with backouts/uplifts on 2 branches. Cam or baku can you take this on?

Comment 9

[Jet Villegas (inactive)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #6)
> jet, is this something you want to weigh in on ?  How much of a performance
> regression is acceptable here?

CSS variables can introduce some overhead when used. This is worse for variables that resolve to images (see bug 1199054.) If you can avoid hitting that case, we may be able to get past most of this Talos regression. Otherwise, let's do as Andrea suggests in comment 5 and not let this code run until the feature is enabled by default. I'd still like to get the test case boiled down to the root cause so I can get that part fixed.

Comment 10

[Andrea Marchesini [:baku]]

Mike, any idea here?
What about if we style the elements 1 by 1 without a template system as we currently do?

I'm OK to disable all until we enable containers by default, but it's just a temporary solution.

Comment 11

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

(In reply to Andrea Marchesini (:baku) from comment #10)
> Mike, any idea here?
> What about if we style the elements 1 by 1 without a template system as we
> currently do?
> 
> I'm OK to disable all until we enable containers by default, but it's just a
> temporary solution.

Let's disable until we enable containers by default to clear the regression and give ourselves some breathing room.

It'd be nice if we could get get a profile for someone to look at to see where the time is going. Considering that tpaint was affected by this, profiles from that talos test should be illuminating.

If we cannot figure out how to improve performance with the CSS variable / templating system we've set up, then let's revert back to the hardcoded values until we can find a proper, performant solution. Sound good?

Comment 12

[Andrea Marchesini [:baku]]

Attachment: css.patch

Comment 13

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

Wait, I thought we were going to go back to the hard-coded values?

Comment 14

[Andrea Marchesini [:baku]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #13)
> Wait, I thought we were going to go back to the hard-coded values?

Well, I'm already working on about:containers and all the creation/editing of them.
This is patch 'fixes' the regression but without loosing the feature that I need for the rest of my work.

Comment 15

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

Comment on attachment 8759702 [details] [diff] [review]
css.patch

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

Okay, let's take it. Thanks baku!

::: browser/components/contextualidentity/ContextualIdentityService.jsm
@@ +67,2 @@
>  
> +    let color = identity ? identity.color : DEFAULT_TAB_COLOR;

Alright, we'll take this for now - but can you please add documentation to this block about how this inline styling is only a temporary measure while we sort out the CSS var perf issues? Also, can you please file a follow-up bug for switching back to CSS var?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca1f81b4d96b
Containers tabs use inline CSS styling because of performances issues with CSS vars, r=mconley

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

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

Just a heads up that we'll probably want to uplift this.

Comment 19

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

(Assuming that the perf regression has gone away)

Comment 20

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

verified this is fixed by manually inspecting the graphs, thanks for the fix!

Comment 21

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

:lizzard, how can we track that this is on the radar to get uplifted

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8759702 [details] [diff] [review]
css.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: lower startup performance caused by the use of CSS variables for containers.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none. We just do inline CSS styling.
[String/UUID change made/needed]: none

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8759702 [details] [diff] [review]
css.patch

Perf fix, please uplift to aurora.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e0da663d6fe5

Comment 25

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

I see some wins on aurora with this!

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

4.2% tpaint osx
4.5% tpaint win8