1133757 - 3-6% Linux 32/Win*/MacOS* cart regression on Mozilla-Inbound (v.38) on Feb 17, 2015 from push a9dd7c71cc68

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, defect)

Description

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

Talos has detected a Firefox performance regression from your commit a9dd7c71cc68 in bug 960465.  We need you to address this regression.

This is a list of all known regressions and improvements related to your bug:
http://alertmanager.allizom.org:8080/alerts.html?rev=a9dd7c71cc68&showAll=1

On the page above you can see Talos 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, 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 linux,win64,win32,macosx64 -u none -t svgr  # add "mozharness: --spsProfile" to generate profile data

To run the test locally and do a more in-depth investigation, first set up a local Talos environment:
https://wiki.mozilla.org/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 cart

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 will be backed out! ***

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

Comment 1

[David Baron :dbaron:]

I'd prefer not to back this out; it's a large set of changes that are needed to match the current spec, and which I think are also needed to get OMT Animations (which should help performance) in better quality to be able to ship on desktop.

I'd be interested in seeing the difference between profiles in the before and after states, although I don't know if we have tools to actually view a difference between profiles.

I'll try to have a look this week to see if there's anything that can be done in the short term, but if I don't get to it this week, it won't be until the week after next.

Comment 2

[David Baron :dbaron:]

(In reply to Joel Maher (:jmaher) from comment #0)
> 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 linux,win64,win32,macosx64 -u none -t svgr  # add "mozharness:
> --spsProfile" to generate profile data

is svgr really the right test here?

Comment 3

[David Baron :dbaron:]

Assuming it is, here's a profiling try run with the push (i.e., on a9dd7c71cc68):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0421c364e24f
and another one without the push (i.e., on 4b118b959ffd):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=30dc5e54c613

Comment 4

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

yes, the cart test runs in the svgr job!  do ask if you have questions- and a timeline of 1-2 weeks is fine.  Keep in mind this will uplift to Aurora (next Monday), so if we do make fixes we should consider them on Aurora as  well.

Comment 5

[David Baron :dbaron:]

I did have some time to investigate briefly, but I'll look more week after next.

Some observations:

After the change, we were spending substantially more time (near or around double) in:

 AncestorFilter::PushAncestor
 ElementRestyler::Restyle
 SearchTable, MapRuleInfoInto, graphite2::vm::Machine::run, PL_HashTableRawLookup

but we were not spending more time in:
 
 FindChildWithRules
 SelectorMatches
 DidSetStyleContext


(This makes me wonder if the regression would be smaller with bug 931668 relanded.)

Comment 6

[David Baron :dbaron:]

It's possible this is bug 1189598.

Comment 7

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

this is a pretty old regression, maybe it is somewhat related to bug 1189598.  Honestly with this being so old, we have shipped the regression.  If there is anything I could do to help with this, let me know!

Comment 8

[Mike Conley (:mconley) (:⚙️)]

We stopped measuring the customize transition when we stopped animating it. CART is no longer a thing. Closing as INVALID.