1267323 - 5.21 - 5.36% damp (windows7-32, windows8-64) regression on push 0361b7030d36 (Fri Apr 22 2016)

Status: RESOLVED WONTFIX

(DevTools :: General, defect, P1)

Description

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

Talos has detected a Firefox performance regression from push 0361b7030d36. 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=997

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#DAMP

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 win32,win64 -u none -t g2-e10s[Windows 7,Windows 8] --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 damp --e10s

Making a decision:
As the patch author we need your feedback to help us handle this regression.

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

Comment 1

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

here are the subtests for windows 8:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=fx-team&originalRevision=005449bf12df&newProject=fx-team&newRevision=0361b7030d36&originalSignature=1c9b4b6e159a90b8868b22cb962acd1e4d9d69b1&newSignature=1c9b4b6e159a90b8868b22cb962acd1e4d9d69b1&framework=1

this only seems to show up on e10s.

:ntim, can you take a look at this and figure out why this is happening and if there is anything we can do to reduce this regression in our test (or help justify if this is worth keeping)?

Comment 2

[Brian Grinstead [:bgrins]]

The changeset is mostly adding firebug icons which shouldn't be touched by DAMP (which should be loading just the light theme).  The exception is changes to toolbars.css that are extracting some hardcoded images into CSS variables: https://hg.mozilla.org/integration/fx-team/rev/0361b7030d363459416d46e651a8c50c9536788f#l42.2.

I'm not sure if there's anything we can do about that, it reminds me of how I wasn't able to extract some of the australis tabs values into variables in https://bugzilla.mozilla.org/show_bug.cgi?id=1122726.  I don't want to have to back out our CSS refactoring though, so we may have to live with the regression unless if Tim / Honza can think of anything to do.

Comment 3

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> The changeset is mostly adding firebug icons which shouldn't be touched by
> DAMP (which should be loading just the light theme).  The exception is
> changes to toolbars.css that are extracting some hardcoded images into CSS
> variables:
> https://hg.mozilla.org/integration/fx-team/rev/
> 0361b7030d363459416d46e651a8c50c9536788f#l42.2.
> 
> I'm not sure if there's anything we can do about that, it reminds me of how
> I wasn't able to extract some of the australis tabs values into variables in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1122726.  I don't want to have
> to back out our CSS refactoring though, so we may have to live with the
> regression unless if Tim / Honza can think of anything to do.
We could try to only set the variables once on `:root` for both dark and light themes (since they share the same images). Right now, they're set twice for theme-light/theme-dark with the same value. I doubt that will change anything perf-wise though.

I think we should just accept the regression if my suggestion isn't effective (I highly doubt it is).

Comment 4

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

:bgrins, I will let you make the call here

Comment 5

[Brian Grinstead [:bgrins]]

The most bizarre regression here is damp simple.performance.reload.DAMP opt e10s increasing 749.43% on windows 7: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=fx-team&originalRevision=b813b1e6e7e3288a865965bb9beefbf9d36b4a67&newProject=fx-team&newRevision=b6f71c9b42b9cffc31e346a447bf736130211cf9&originalSignature=0c0bb95cce8f2fc538367aeb786aba2a6381e9ad&newSignature=0c0bb95cce8f2fc538367aeb786aba2a6381e9ad

and 661.36% on windows 8: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=fx-team&originalRevision=b813b1e6e7e3288a865965bb9beefbf9d36b4a67&newProject=fx-team&newRevision=b6f71c9b42b9cffc31e346a447bf736130211cf9&originalSignature=1c9b4b6e159a90b8868b22cb962acd1e4d9d69b1&newSignature=1c9b4b6e159a90b8868b22cb962acd1e4d9d69b1.

Other than that I'd say it's a fairly low startup time penalty in order to use variables

Comment 6

[Brian Grinstead [:bgrins]]

Hm, I think the links from Comment 5 are pointing to a different changeset (and indeed the dashboard at https://treeherder.mozilla.org/perf.html#/alerts?id=997 seems to reference this commit: https://hg.mozilla.org/integration/fx-team/rev/b6f71c9b42b9cffc31e346a447bf736130211cf9).  Joel, can you confirm that the alert #997 which is linked to from Comment 0 is due to 0361b7030d36?

Comment 7

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

this was a hard one to figure out as the data wasn't super clear even with the additional retriggers- the alert was on b6f71c9b42b9, but retriggers and data point at 0361b7030d36.

Comment 8

[Brian Grinstead [:bgrins]]

We won't back out the Firebug theme (and I don't want to make it less maintainable by removing these variables).  I'd like to ask for help optimizing variables on the platform instead of limiting our usage on the frontend.  And if there's no gains to be had there, we'll have to live with the regression

Cam, surely startup time is going to be affected as we move more values into variables.  But we are seeing what looks like a 5% overall regression (larger when measuring just 'toolbox open' time) on Windows only after this push: https://hg.mozilla.org/integration/fx-team/rev/0361b7030d363459416d46e651a8c50c9536788f#l42.2.  Do you have any ideas of things we could test out on Try that might speed it up?  Or any hunches about what might be causing the windows regression?  Thanks in advance

Comment 9

[Brian Grinstead [:bgrins]]

Went ahead and pushed a backout of just the CSS changes to try to confirm it speeds back up:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f76f6a9b137
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=5f76f6a9b137

Comment 10

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

what are the next steps here?  I would like to resolve this one way or another.  any fixes should be considered for aurora as well.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Joel Maher (:jmaher) from comment #10)
> what are the next steps here?  I would like to resolve this one way or
> another.  any fixes should be considered for aurora as well.

I've confirmed that the regression goes away if backing out the variables: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=8c3fd523d75b&newProject=try&newRevision=5f76f6a9b137&framework=1&filter=damp&showOnlyImportant=0.  But that's not practical for reasons outlined in Comment 8, so I think we are going to have to live with the regression.  If the platform can speed us up later on that's great.

Comment 12

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

I say lets close this as wontfix.  Thank you for looking into this- we can't fix everything- lets choose our battles!  Please feel free to close this out if you feel that is the right thing.

Comment 13

[Brian Grinstead [:bgrins]]

Closing out.  We should also consider resolving Bug 1267044 - I don't fully understand if these are completely different issues, they seem related

Comment 14

[Cameron McCormack (:heycam)]

Just to quickly follow up: there are some things I think we can do to speed up variable processing, such as bug 1199054, but I won't have time to look at it soon.