1574844 - 1.65% tp5o_scroll (linux64-shippable-qr) regression on push a9f37a8db55158dbe6d3938202fcc6032d98c00f (Mon July 29 2019)

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/autoland/pushloghtml?changeset=a9f37a8db55158dbe6d3938202fcc6032d98c00f

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

2% tp5o_scroll linux64-shippable-qr opt e10s stylo 2.42 -> 2.46

You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=22524

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/TestEngineering/Performance/Talos

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/TestEngineering/Performance/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

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

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

I took a look at this and I can't immediately see why there might be a talos regression from that change. It was mostly a simple margin/padding update. Additionally, the "separator" has since been re-implemented in a way that's consistent with our other separators.

I can try to do a bit more digging, but in the meantime I do think it's worth investigating whether there are any other candidate patches to blame for this regression.

Comment 2

[Johann Hofmann [:johannh]]

I agree this is weird. If anything I would have expected the new permanent shield icon to show up in talos...

It might be that there's some styling in the ::after element that makes up the separator line that is expensive? We could try to bisect removing rules from there, but as Nihanth mentioned this has also been re-implemented since then...

That regression size is also not super large, so try-bisecting might be hard to impossible after 22 days of heavy changes in between...

Comment 3

[Alexandru Ionescu (needinfo me) [:alexandrui]]

I did some retriggers to double check this. Will come back with updates.

Comment 4

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Unfortunately the retriggers didn't reveal nothing new.

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Okay, I'm really skeptical that the changes to the icon spacing could have caused this talos regression, but what I can try and do is to run tp5o_scroll on two try pushes - one with old styles and one with current mozilla-central styles, and get an up-to-date perf comparison.

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

mozilla-central: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0478e05bebc761bbd77bf26afec372eee2f59c35

Spacing/sizing reverted and border removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19e2f3872b724fe8ab4461c17ab9a063ba7ad3f2

Spacing/sizing reverted, border kept as it is in m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ce7b421275552a7b05ee4381f7b51140431e2d7

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Okay, I'm learning how to run talos tests. Apparently tp5o_scroll is part of talos-g1, not talos-tp5o. So I made another push:

Spacing/sizing reverted and border removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f30953e5cf7afdcd749c28997b7624efd5a177c8

Spacing/sizing reverted, border kept as it is in m-c: https://treeherder.mozilla.org/#/jobs?repo=try&revision=030b1ef87e09b61b6b31461e62171345708c65ab

Should be easy to compare this to m-c tip.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Both try pushes show a tp5o_scroll regression (~2%). Interestingly, removing the separator/border increases the delta! That was a bit strange to me.

Next thing I want to try is remove the position: relative and see what happens. tp5o_scroll performance deltas are correlated to changes that affect the display list[1] and I think position: relative is the only rule that could have impact here.

[1] https://wiki.mozilla.org/TestEngineering/Performance/Talos/Tests#Possible_regression_causes_2

Comment 9

[(inactive) Nihanth Subramanya [:nhnt11]]

Removed position: relative; and no separator: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1741bd3993df8a7176468e12a5939fe35fef831d

Removed position: relative; and kept the separator: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d17ade8bc5469a23aa9ddceae8a24ee032e8f66

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

OK! Every single one of these pushes shows a regression compared to mozilla-central. In particular, the first link in comment 9 ("Removed position: relative; and no separator") effectively backs out all of the changes in the suspect changeset.

This means that backing out the suspect patch will actually cause further regression. Whatever the issue is here, it seems that the suspect patch is not guilty.

I don't have any more ideas, please let me know if I can help further here.

Comment 11

[Alexandru Ionescu (needinfo me) [:alexandrui]]

Hi Nihanth! Looks like the issue was fixed in bug 1570248 and --lwt-background-tab-separator-color was the cause of the regression. I'm closing this as fixed.
Thank you for your effort!