1596416 - 2.01 - 3.44% tart (linux64-shippable, windows10-64-shippable, windows10-64-shippable-qr, windows7-32-shippable) regression on push 59dcb38e46932d516d5a704795900e13d0a9b41d (Tue November 12 2019)

Status: RESOLVED WONTFIX

(Core :: Layout, defect, P1)

Description

[Florin Strugariu [:Bebe]]

Talos has detected a Firefox performance regression from push:

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

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

Regressions:

3% tart windows10-64-shippable-qr opt e10s stylo 2.41 -> 2.49
3% tart windows7-32-shippable opt e10s stylo 2.72 -> 2.81
3% tart windows10-64-shippable opt e10s stylo 2.72 -> 2.80
2% tart linux64-shippable opt e10s stylo 2.26 -> 2.30

Improvements:

5% about_preferences_basic windows7-32-shippable opt e10s stylo 139.21 -> 132.76

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

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

[Brian Grinstead [:bgrins]]

Interesting this also sped up about:preferences at the same time. Would it be possible to get gecko profiles with tart on the affected platforms before/after the change?

Comment 2

[Tim Nguyen :ntim]

Attachment: Bug 1596416 - Set min-height: 0; on .tab-stack children.

This is cheaper for layout to compute on grid items than the default min-height: auto;

Comment 3

[Daniel Holbert [:dholbert]]

Note: I spammed some retriggers for the talos test-tasks that had reports here, for the start/end of the range, so that we'd have 5+ samples before and after, rather than 1 before and 1 after. This adds some credibility to the regression & gives Perfherder's a bit more info for its analysis.

As a side effect of this, we got some additional tscrollx and tsvgx runs (because those are part of the same task as tart so they rode along with the retriggers), and it seems that now PerfHerder thinks we had improvements for those talos suites as well (based on the new data at least).

Comment 4

[Florin Strugariu [:Bebe]]

gecko profiles:
https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=5e6b050c9427da9ae37ca4c7c8000ba5b47ceea8&tochange=1a85c571a4645798ba4d9295ba2504acac149ac8&searchStr=t%28s-p%29

Comment 5

[Tim Nguyen :ntim]

I've just landed bug 1596966 which should slightly help.

Florin, is it expected that the profiles have the status "testfail" ? Also, do you know how can I open them into https://profiler.firefox.com/ ?

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen :ntim from comment #5)

I've just landed bug 1596966 which should slightly help.

Florin, is it expected that the profiles have the status "testfail" ? Also, do you know how can I open them into https://profiler.firefox.com/ ?

Also Florin, could you please check if bug 1596966 improved things?

Comment 7

[Florin Strugariu [:Bebe]]

I have not seen any performance changes for the pinged bug 1596966

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Florin Strugariu [:Bebe] (needinfo me) from comment #7)

I have not seen any performance changes for the pinged bug 1596966

We were hoping Bug 1596966 would help things but it didn't appear to. The good news is that since we are now using CSS grid, this should incrementally improve as grid perf improves. The bad news is that I don't know what else to try to handle this 2-3% tart regression.

Mike, I don't know how much more time we should spend on this. Would you be OK with taking this regression? Also ni? Daniel to see if there are other ideas on the layout side that could help things in the short term.

Comment 9

[Daniel Holbert [:dholbert]]

Also ni? Daniel to see if there are other ideas on the layout side that could help things in the short term

(I don't have any easy/short-term ideas for this, unfortunately.)

Comment 10

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

Sorry for the delay.

I'm afraid the performance profiles in comment 4 didn't work out. I've re-pushed to see if there's anything that sticks out:

Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e37554c5750cc14586ff67bee283af9e50563726
Patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d58dae924c1d797fd40439a79b53d6d1bba2077

Keeping needinfo on self until I can examine and see if there's anything actionable here. If not, we might just have to eat this one. :/

Comment 11

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

Huh, I wasn't able to notice a difference between the builds using compare chooser...

Decided to re-push on all platforms yesterday. Still waiting for Windows jobs to run.

Baseline: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfc91918be82ae06f8b7ec8ea8454228e4fa500b
Patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=661ba4e8004762f33b21a7c56d452c27ee962bc0

Comment 12

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

Sorry for the delay, finally got profiles here.

The problem with the TART test is that it measures things that are very small - the time it takes to rasterize and present frames for tab animations in ASAP mode. These tests are notoriously sensitive, and it's oftentimes difficult to derive much actionable data out of a performance profile for them because the regressions that it detects are often too small for the profiler to highlight.

I will note, however, that it seems as if we've got more samples being collected in the "bad" case inside of nsGridContainerFrame::Reflow as opposed to the time spent inside of nsStackLayout::XULLayout for the "good" case.

This makes me hypothesize that doing grid layout has slightly more overhead than stack layout.

We should continue to do performance optimizations on our grid implementation, but I honestly don't think there's really much that's directly actionable here. :( I think we'll just have to take it.