1122726 - Convert tabCurveWidth and tabCurveHalfWidth into CSS variables

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Brian Grinstead [:bgrins]]

In Bug 1091001 we ended up reverting the tabCurveWidth and tabCurveHalfWidth variable changes due to what looked like a performance regression on try pushes (Comments 19 and 20).

Will try to tackle that separately to see if it helps narrow down the specific problem.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: tab-curve-width.patch

Patch that converts the preprocessed values to variables.  Also includes a workaround for the shorthand margin property (Bug 1099448)

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8550499 [details] [diff] [review]
tab-curve-width.patch

Matt, I'm not seeing a regression anymore [0], so requesting review.  This is basically the part from the patch in Bug 1091001 that got stripped out because of a suspected tart regression (Bug 1091001 Comment 20).

[0]: http://compare-talos.mattn.ca/dev/?oldBranch=Fx-Team&oldRevs=4bb30ec57818&newBranch=Try&newRev=871145c95a52&submit=true

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 8550499 [details] [diff] [review]
tab-curve-width.patch

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

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Matt, I'm not seeing a regression anymore [0], so requesting review. 

I wonder why that is…

Comment 4

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=149043206ad3

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/34d0394ad69d

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/34d0394ad69d

Comment 7

[Brian Grinstead [:bgrins]]

Looks like this is causing a tart regression on linux (see details below).

Joel, given the tart regression, do you think we should back this out until we come up with a performance fix or let it stick?  I suspect there is some non-zero overhead to switching to css variables regardless of how much they are optimized, but am unsure of how much could be tolerated in this test / platforms.

Regression: Fx-Team-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 x64 - 6.95% increase
---------------------------------------------------------------------------------------
    Previous: avg 4.274 stddev 0.053 of 12 runs up to revision fcd8ddf5cd31
    New     : avg 4.571 stddev 0.056 of 12 runs since revision 34d0394ad69d
    Change  : +0.297 (6.95% / z=5.585)
    Graph   : http://mzl.la/15KAHZk

Changeset range: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=fcd8ddf5cd31&tochange=34d0394ad69d


Regression: Fx-Team-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 - 5.53% increase
-----------------------------------------------------------------------------------
    Previous: avg 5.043 stddev 0.041 of 12 runs up to revision fcd8ddf5cd31
    New     : avg 5.321 stddev 0.030 of 12 runs since revision 34d0394ad69d
    Change  : +0.279 (5.53% / z=6.723)
    Graph   : http://mzl.la/15KIbvB

Changeset range: http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=fcd8ddf5cd31&tochange=34d0394ad69d

Comment 8

[Brian Grinstead [:bgrins]]

Cam, I had cleared the needinfo regarding css variables performance when we closed Bug 1088771, but this is a similar situation (see Comment 7).  Switching to variables in the tabs is causing a regression in the tab animation tests - do you have any ideas how to improve this?

Relevant changes are here: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1122726&attachment=8550499.  There also had to be some calc() calls added to negate the variable value - not sure if that is contributing to the issue.

Comment 9

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

bgrins, thanks for checking- I saw these come through but didn't get a chance to investigate.  It appears to be linux* TART, and since both are >5%, it is worth taking a brief look into.

Do you have bandwidth to investigate a possible fix (or something to reduce the regression)?  This is a trade-off of how much time to investigate vs the benefits of the code landing.  

If these were 1/2 the regression (i.e. ~2.5-3%), I would say leave it alone and document why we think the regression happened.  Since this is higher, I would be inclined to do some basic investigation (i.e. not days, but a couple hours at the most).  Looking at the patch, there is a comment to adjust something in the near future:
https://hg.mozilla.org/integration/fx-team/rev/34d0394ad69d#l1.41

will that affect performance?  Do we get a lot of value from this change in general?

This is in 2 subtests:
simple-close-DPI1.all.TART
simple-open-DPI1.all.TART

Avi, with these two tests ^, what do you think is the perceived user impact?

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Joel Maher (:jmaher) from comment #9)
> Looking at the patch, there is a comment to
> adjust something in the near future:
> https://hg.mozilla.org/integration/fx-team/rev/34d0394ad69d#l1.41
> 
> will that affect performance?

That's Bug 1099448, and it has to do with incorrect parsing of shorthand properties that include variables (and some other conditions).  Cam would be a better person to answer whether that is likely to affect performance.

> Do we get a lot of value from this change in general?

The benefit is taking care of code debt, so that we can remove duplicate code and selectors in Bug 1095795, specifically: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1095795&attachment=8554892.  So it's not urgent, but I wouldn't want it to get dropped either, since the duplication could bite us if someone makes a change to tabs.inc.css.

Comment 11

[Avi Halachmi (:avih)]

So, while trying to evaluate the perf impact, I've used compare talos to compare changesets 63f8a47cfdf5 (new) to 232401a6d1cc (old): http://compare-talos.mattn.ca/?oldRevs=232401a6d1cc&newRev=63f8a47cfdf5&server=graphs.mozilla.org&submit=true

These are apparently the two m-c batches which got tested, new includes this patch, and old doesn't include the patch.

Scrolling down a bit to TART, it shows 7.5% regression on OS X 10.8, ~3% on linux, and ~2% on windows platforms.

Clicking the details link of the OS X regression shows that while everything regressed, the biggest regression appears to be when testing with 2x DPI (twice as big tabs and content rendering): http://compare-talos.mattn.ca/breakdown.html?oldTestIds=44739790&newTestIds=44740984&testName=tart&osName=Mac%2010.8&server=graphs.mozilla.org

Brian, would you expect this patch to have different impact on different platforms? is the patch only applied on some desktop platforms?

Any idea why the biggest impact is with 2x DPI?

So typically, for normal DPI, it seems that the regression still exists but is lower than the overall average of 7.5% (on OS X 10.8), probably about 4% on average (rough estimation).

Since I've never looked at TART running on retina display, and since I don't know if our talos OS X test machines have retina, It's harder for me to estimate the impact of this on retina displays.

But other than retina, according to the numbers I'm looking at from the links above, the impact exist but is relatively small, possibly worth the better code quality.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Avi Halachmi (:avih) from comment #11)
> So, while trying to evaluate the perf impact, I've used compare talos to
> compare changesets 63f8a47cfdf5 (new) to 232401a6d1cc (old):
> http://compare-talos.mattn.ca/
> ?oldRevs=232401a6d1cc&newRev=63f8a47cfdf5&server=graphs.mozilla.
> org&submit=true
> 
> These are apparently the two m-c batches which got tested, new includes this
> patch, and old doesn't include the patch.
> 
> Scrolling down a bit to TART, it shows 7.5% regression on OS X 10.8, ~3% on
> linux, and ~2% on windows platforms.
> 
> Clicking the details link of the OS X regression shows that while everything
> regressed, the biggest regression appears to be when testing with 2x DPI
> (twice as big tabs and content rendering):
> http://compare-talos.mattn.ca/breakdown.
> html?oldTestIds=44739790&newTestIds=44740984&testName=tart&osName=Mac%2010.
> 8&server=graphs.mozilla.org
> 

It looks like the compare link only has one run of tart for each platform so it could just be noise - I retriggered the suites 20 times for each revision (svgr and other, since I couldn't remember which one it is in).

> Brian, would you expect this patch to have different impact on different
> platforms? is the patch only applied on some desktop platforms?

It's applied on all platforms, but I've seen regressions that only affected one or two platforms when switching to variables before, so that wouldn't be too surprising.

Comment 13

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

hmm, this is showing up on more platforms now that we have all our data:
http://alertmanager.allizom.org:8080/alerts.html?rev=34d0394ad69d&table=1

I see windows xp and 7, in addition to linux*.

There were a series of revisions in this push:
https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=34d0394ad69d

Only one other could possibly adjust stuff:
https://hg.mozilla.org/integration/fx-team/rev/750f76c08fec

I am more inclined to call a backout, but I shouldn't be the stakeholder to make that call :)

Comment 14

[Brian Grinstead [:bgrins]]

Backed out 34d0394ad69d: https://hg.mozilla.org/integration/fx-team/rev/315321e5b7d6

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: tab-curve-width.patch

Rebased and with a fresh try push on perfherder (fingers crossed): https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=d4f3a8a75577&newProject=try&newRevision=a95a263f36be

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Created attachment 8646702 [details] [diff] [review]
> tab-curve-width.patch
> 
> Rebased and with a fresh try push on perfherder (fingers crossed):
> https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-
> central&originalRevision=d4f3a8a75577&newProject=try&newRevision=a95a263f36be

Well, looks like it's detected a 2% windowsxp tpaint regression and ~2% tart regressions on linux32, windows7-32, and windowsxp.  At least it's better than before (around 6-7%, see Comment 7)

Comment 17

[Brian Grinstead [:bgrins]]

New talos push with the rebased patch: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=bd67f59e818cce52a2b9a4a80e4e5da4d94fe254&newProject=try&newRevision=eeb42aa80b3767e953dc5d523901795d77b2940d&framework=1&showOnlyImportant=0.

The only thing I see so far is 2.33% on tsvgx opt e10s osx, but windows results haven't come in.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: tab-curve-width-var.patch

The try push actually looks good - I guess whatever caused the TART regression has been fixed!

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a98388a60bd
Convert tabCurveWidth and tabCurveHalfWidth into CSS variables;r=MattN

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a98388a60bd

Comment 21

[Brian Grinstead [:bgrins]]

Clearing needinfo since this got landed