1088771 - CSS variable usage within browser.css on MacOSX 10.8 causes a 2.12% increase in Customization Animation Tests

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect)

Description

[Brian Grinstead [:bgrins]]

More information in the message below.


Regression: Fx-Team - Customization Animation Tests - MacOSX 10.8 - 2.63% increase
----------------------------------------------------------------------------------
    Previous: avg 35.577 stddev 0.307 of 12 runs up to revision 3df00a905527
    New     : avg 36.513 stddev 0.395 of 12 runs since revision a890f2283b74
    Change  : +0.936 (2.63% / z=3.044)
    Graph   : http://mzl.la/1tsq7iJ

Changeset range: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=3df00a905527&tochange=a890f2283b74

Changesets:
  * http://hg.mozilla.org/integration/fx-team/rev/a890f2283b74
    : Brian Grinstead <[e-mail]> - Bug 1053434 - Switch some hardcoded OSX values to CSS variables for devedition;r=Gijs
    : http://bugzilla.mozilla.org/show_bug.cgi?id=1053434

Bugs:
  * http://bugzilla.mozilla.org/show_bug.cgi?id=1053434

Comment 1

[Brian Grinstead [:bgrins]]

Cameron, I'm not sure how to tackle this, any ideas?  I have similar patches in review right now for windows and linux that I was planning to land within the next week.

Comment 3

[Brian Grinstead [:bgrins]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1088844#c0:

Here is a graph showing the issue:
http://graphs.mozilla.org/graph.html#tests=%5B%5B309,64,24%5D%5D&sel=1413966342034.9492,1414133284407.8306&displayrange=7&datatype=running

and a view on tbpl with some retriggers:
https://tbpl.mozilla.org/?tree=Fx-Team&fromchange=5a7d636882fe&tochange=6cc96113e8a3&jobname=Rev5%20MacOSX%20Mountain%20Lion%2010.8%20fx-team%20talos%20svgr

you can see that the values for CART stay above 36.0 (really 36.5) after this push:
https://hg.mozilla.org/integration/fx-team/rev/a890f2283b74

Comment 4

[Brian Grinstead [:bgrins]]

I don't know how to read this data, is the problem still persisting after a few days of new tests?  If so, will we need to back out the change assuming that the variable performance can't be optimized in time for the release?

Comment 5

[:Gijs (he/him)]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> I don't know how to read this data, is the problem still persisting after a
> few days of new tests?

You can click the graph link in comment #0 and switch to 30 days, and you can see the increase (however small it is). It doesn't seem to have magically gone away yet, no.

> If so, will we need to back out the change assuming
> that the variable performance can't be optimized in time for the release?

Unsure. 2.1% on this particular test isn't as bad as it could be. I'd be worried if tpaint or ts_paint is also affected. I also don't really understand /why/ this particular test is affected - I'd expect tpaint/ts_paint to suffer more. I also don't know what our perf optimization range is, ie how easy it is to fix the performance here.

I'm also interested as to whether the effects on Windows and Linux are going to be as bad...

Comment 6

[Joe Walker [:jwalker]]

Summary from email:

Jet:

> I'm seeing a lot of noise in the test graph:
> http://graphs.mozilla.org/graph.html#tests=[[309,64,24]]&sel=1413966342034.9492,1414133284407.8306&displayrange=7&datatype=running
>
> ie. we go up and down to the perf levels before the CSS vars were added. 
> 
> I'd expect some overhead from CSS vars, and this change looks like it's
> within the noise threshold. Are you guys seeing other perf test regressions? 

Brian:

> This is the only regression that I've been notified of.
> According to https://bugzilla.mozilla.org/show_bug.cgi?id=1088771#c5
> this particular test is less worrisome than if it were tpaint or tspaint.
> It's sounding like we should proceed with landing the windows/linux
> variations of this patch and see if there is a similar regressions.
>
> If so, do you think we would be OK for these to stick and even be uplifted
> to Aurora?

Jet:

> I think so, but please keep the perf bug open as we'd still like to see if
> Cam finds a fix for us when he gets back in November. Drop me a line if
> you see other fallout from this change, and we can run another profile.

Comment 7

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

looking at other platforms:
http://graphs.mozilla.org/graph.html#tests=%5B%5B309,64,24%5D,%5B309,132,33%5D,%5B309,132,25%5D,%5B309,132,35%5D%5D&sel=1413898496247,1414503296247&displayrange=7&datatype=running

we see win7, linux32 and linux64 to have problems as well.  The win7 regression seems so small though.  Note, win8 had a hiccup with results due to a 32 vs 64 and pgo vs nonpgo switch, so I am ignoring any win8 regressions until next week.

As it stands, this appears to be on trunk builds only, not on Aurora (unless I am mistaken somehow).

Even a small fix here would be helpful as we have a series of small cart regression in v.36.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Joel Maher (:jmaher) from comment #7)
> As it stands, this appears to be on trunk builds only, not on Aurora (unless
> I am mistaken somehow).

To be clear, these are not already on Aurora - we are going to want to uplift these patches to Aurora soon, most likely this week.

Comment 10

[Brian Grinstead [:bgrins]]

Cam, just curious if you may have a chance to look at this?  I have another patch I'd like to land that converts more things to CSS variables for Bug 1091001, but I'm seeing what looks like a cart regression on the try push:  http://compare-talos.mattn.ca/dev/?oldBranch=Fx-Team&oldRevs=94be697bd801&newBranch=Try&newRev=a156536eadd1&submit=true

Comment 11

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

I do not see this on mozilla-beta, can we close this?

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Joel Maher (:jmaher) from comment #11)
> I do not see this on mozilla-beta, can we close this?

I've still bumped into regressions when trying to land a patch that converts more values to variables - see Bug 1091001 Comment 19 and Comment 20.  I ended up just undoing some of the changes until the regression wasn't showing up on a try push.

If this is no problem on beta though, I'd say we can close it.  I can open a new bug to deal with those specific performance issues I was seeing.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> If this is no problem on beta though, I'd say we can close it.  I can open a
> new bug to deal with those specific performance issues I was seeing.

Filed Bug 1122726.  I will re-run talos with that patch and ni? Cam if I'm still seeing regressions there.