1270515 - 2.49 - 3.89% cart / tart (linux64, windows7-32, windowsxp) regression on push 581279cd287e34b4af6b2544daff4589b6a2e580 (Wed May 4 2016)

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[William Lachance (:wlach)]

Hi Bobby, could you investigate this?

jmaher generated a compare view with a bunch more information on what regressed.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=374889c1ff7d&newProject=mozilla-inbound&newRevision=581279cd287e&framework=1

Looking at the subtests for win7 this looks like an across-the-board cart regression (haven't diagnosed tart):

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=374889c1ff7d&newProject=mozilla-inbound&newRevision=581279cd287e&originalSignature=6c4052da1f625e6fc89a169fd288e723db7b73b6&newSignature=6c4052da1f625e6fc89a169fd288e723db7b73b6&framework=1

--

Talos has detected a Firefox performance regression from push 581279cd287e34b4af6b2544daff4589b6a2e580. 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=1103

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#TART.2FCART

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,linux64 -u none -t svgr[Windows 7,Windows XP],svgr-e10s[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 tart:cart

(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 Monday, 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

Comment 1

[Bobby Holley (:bholley)]

So the issue here is that we end up creating various nsStyleCoord temporaries each time we call nsStyleSides::Get, and that creating those temporaries is more expensive than it should be because it involves an unnecessary call to Reset. I have patches for both issues.

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Avoid synthesizing temporary nsStyleCoords in nsStyleSides::ConvertsToLength. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Avoid synthesizing temporary nsStyleCoords in nsStyleSides::ToLength. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Bonus fix: Factor out unnecessary Reset calls during nsStyleCoord initialization. v1

The previous patches prevent us from generating temporaries at the affected callsites,
but this could still have benefits in other places.

Comment 5

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed357ec41846&selectedJob=20439517

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8749486 [details] [diff] [review]
Part 1 - Avoid synthesizing temporary nsStyleCoords in nsStyleSides::ConvertsToLength. v1

>+  // Callers must verify IsCalcUnit before calling this routine.
>+  static Calc* AsCalcValue(nsStyleUnion aValue) {
>+    return static_cast<Calc*>(aValue.mPointer);
>+  }
>+

Could you move this down next to all the Get*Value declarations?

Please either replace "routine" with "method" or "function".

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/56ebc38830b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/abe9894a40eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/332bd75b5e97

Comment 8

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

thanks, I see some wins already!
https://treeherder.mozilla.org/perf.html#/alerts?id=1124

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/56ebc38830b4
https://hg.mozilla.org/mozilla-central/rev/abe9894a40eb
https://hg.mozilla.org/mozilla-central/rev/332bd75b5e97

Comment 10

[Bobby Holley (:bholley)]

Joel, can you confirm we're all good here?

Comment 11

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

all good, a couple lines are not back to normal as there was 2 regressions in a row on the tree (oh fun times), but for every single graph of the original regression there is a corresponding improvement.

Comment 12

[Bobby Holley (:bholley)]

\o/