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

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?

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.

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.

Tracking for 49 since this is a recent regression.

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.

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

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

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?

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

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.

(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?

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

(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 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?

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

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

(Assuming that the perf regression has gone away)

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

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

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 on attachment 8759702 [details] [diff] [review] css.patch Perf fix, please uplift to aurora.

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