Closed Bug 1273628 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Security, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 + fixed
firefox50 --- fixed

People

(Reporter: jmaher, Assigned: baku)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression][domsecurity-active])

Attachments

(1 file)

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?
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini) → needinfo?(cam)
jet, is this something you want to weigh in on ?  How much of a performance regression is acceptable here?
Flags: needinfo?(bugs)
(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.
Component: Untriaged → DOM: Security
Product: Firefox → Core
Version: 47 Branch → 49 Branch
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.
Flags: needinfo?(cam)
Flags: needinfo?(bugs)
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini) → needinfo?(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?
Flags: needinfo?(mconley)
Attached patch css.patchSplinter Review
Assignee: nobody → amarchesini
Attachment #8759702 - Flags: review?(mconley)
Wait, I thought we were going to go back to the hard-coded values?
Flags: needinfo?(amarchesini)
(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.
Flags: needinfo?(amarchesini)
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?
Attachment #8759702 - Flags: review?(mconley) → review+
Blocks: 1278177
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
Whiteboard: [talos_regression] → [talos_regression][domsecurity-active]
https://hg.mozilla.org/mozilla-central/rev/ca1f81b4d96b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Just a heads up that we'll probably want to uplift this.
Flags: needinfo?(amarchesini)
(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
Flags: needinfo?(lhenry)
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
Flags: needinfo?(amarchesini)
Attachment #8759702 - Flags: approval-mozilla-aurora?
Comment on attachment 8759702 [details] [diff] [review]
css.patch

Perf fix, please uplift to aurora.
Flags: needinfo?(lhenry)
Attachment #8759702 - Flags: approval-mozilla-aurora? → approval-mozilla-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
You need to log in before you can comment on or make changes to this bug.