1508213 - 4.39 - 5.64% sessionrestore / ts_paint / ts_paint_webext (windows10-64-qr) regression on push c7a9ceb9becbccaf75f65eec47a98f0a72fafb02 (Fri Nov 16 2018)

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Ionuț Goldan [:igoldan]]

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c7a9ceb9becbccaf75f65eec47a98f0a72fafb02

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

Regressions:

  6%  ts_paint windows10-64-qr opt e10s stylo            341.58 -> 360.83
  6%  sessionrestore windows10-64-qr opt e10s stylo      340.00 -> 358.83
  4%  ts_paint_webext windows10-64-qr opt e10s stylo     355.08 -> 370.67


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

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/Performance_sheriffing/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Performance_sheriffing/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/Performance_sheriffing/Talos/RegressionBugsHandling

Comment 1

[Ionuț Goldan [:igoldan]]

I'll soon post the Gecko profiles.

Comment 2

[Ionuț Goldan [:igoldan]]

Here are the Gecko profiles for ts_paint on Windows 10 QR:

before: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FCA55Vdn8Ql2bsibJjHmgXA%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_ts_paint.zip

after: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FFFvV5nCcTB2U7wdQjWZwyQ%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_ts_paint.zip


For sessionrestore on Windows 10 QR:

before: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FCA55Vdn8Ql2bsibJjHmgXA%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_sessionrestore.zip

after: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FFFvV5nCcTB2U7wdQjWZwyQ%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_sessionrestore.zip


For ts_paint_webext on Windows 10 QR:

before: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FQjLaWdPeTViVHiaLofm5Og%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_ts_paint_webext.zip

after: https://perf-html.io/from-url/https%3A%2F%2Fqueue.taskcluster.net%2Fv1%2Ftask%2FNrUSbzQDQs2Smp9Ugluogw%2Fruns%2F0%2Fartifacts%2Fpublic%2Ftest_info%2Fprofile_ts_paint_webext.zip

Comment 3

[Brian Grinstead [:bgrins]]

Interesting that this is QuantumRender only. I've been requested to skip those on try pushes to save resources which is why this was missed before landing.

I seem to remember that QR has some slow paths related to trees, and I do see a very slow getter on `columns` (24ms / https://perfht.ml/2A2OtrO), which returns treeBoxObject.columns: https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/content/widgets/tree.xml#41-42.

We could lazify this some more for this particular case (since the trees are not visible), but ultimately I'm assuming this lookup will need to be optimized in order to get QR to parity on other tests that do display trees (like about_preferences_basic). Jeff, do you have any more details about slow path for trees and any plans to mitigate it to reach perf parity?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> Interesting that this is QuantumRender only. I've been requested to skip
> those on try pushes to save resources which is why this was missed before
> landing.
> 
> We could lazify this some more for this particular case (since the trees are
> not visible), but ultimately I'm assuming this lookup will need to be
> optimized in order to get QR to parity on other tests that do display trees
> (like about_preferences_basic). Jeff, do you have any more details about
> slow path for trees and any plans to mitigate it to reach perf parity?

The slowness here on treeBoxObject.columns doesn't seem specific to trees.
.columns is causing a restyle which is causing us to create the compositor for the first time.
Did this change move around the initialization of things?

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > Interesting that this is QuantumRender only. I've been requested to skip
> > those on try pushes to save resources which is why this was missed before
> > landing.
> > 
> > We could lazify this some more for this particular case (since the trees are
> > not visible), but ultimately I'm assuming this lookup will need to be
> > optimized in order to get QR to parity on other tests that do display trees
> > (like about_preferences_basic). Jeff, do you have any more details about
> > slow path for trees and any plans to mitigate it to reach perf parity?
> 
> The slowness here on treeBoxObject.columns doesn't seem specific to trees.
> .columns is causing a restyle which is causing us to create the compositor
> for the first time.
> Did this change move around the initialization of things?

Oh, quite possibly. I think we can delay this for now and wait for the tree to get constructed due to layout. Although eventually when we migrate the tree binding to Custom Element we won't get that layout hook anymore and will end up needing to run some of this code earlier on. At what point do we expect to / want to create the compositor during startup? We can look into pushing tree construction back if there's a specific event / point in time we can wait for.

Victor - I suspect if we basically skip the call to `this.parentNode` (https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/content/widgets/tree.js#160-161) in the case when `this.isRunningDelayedConnectedCallback`, similar to what we do with treecol (https://searchfox.org/mozilla-central/rev/b03a62c3c82316e733a3b09622c1cb7e59f64cc3/toolkit/content/widgets/tree.js#236-238) that would do the trick. I'm pretty sure we'll also have to then do a call to this._ensureColumnOrder() in the tree constructor - it looks like tree doesn't manage that itself currently, but it'd make sense to do so.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

I'm not exactly sure where it ended up. Dan and Matt have both looked at WebRender startup more recently and can probably give a better answer.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Waiting for the MozAfterPaint should give you a good time to add things that might depend on the compositor being available.

Looks like there's also a WR bug where shader compiling isn't async like it should be. I filed https://github.com/servo/webrender/pull/3329 for that.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Waiting for the MozAfterPaint should give you a good time to add things that
> might depend on the compositor being available.
> 
> Looks like there's also a WR bug where shader compiling isn't async like it
> should be. I filed https://github.com/servo/webrender/pull/3329 for that.

Do you expect your patch to webrender will resolve this without changes on the browser chrome side? Or will this still need to be deferred until MozAfterPaint?

Comment 9

[Matt Woodrow (:mattwoodrow)]

I *think* it will be. I'll try get it landed ASAP, and we can see where we end up.

Comment 10

[Matt Woodrow (:mattwoodrow)]

To be clear, I think that it will be sufficient to resolve the issue, without browser chrome side changes.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Bug 1508348 is in central now, and it looks like the regression is gone.

Probably worth waiting for the official perfherder alert, but I think we should be sorted here.

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Bug 1508348 is in central now, and it looks like the regression is gone.
> 
> Probably worth waiting for the official perfherder alert, but I think we
> should be sorted here.

Thank you!

Comment 13

[Ionuț Goldan [:igoldan]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Bug 1508348 is in central now, and it looks like the regression is gone.
> 
> Probably worth waiting for the official perfherder alert, but I think we
> should be sorted here.

I confirm this got fixed! Even better, ts_paint_webext got a small extra boost.

== Change summary for alert #17694 (as of Tue, 20 Nov 2018 07:59:28 GMT) ==

Improvements:

  6%  ts_paint_webext windows10-64-qr opt e10s stylo     369.33 -> 346.08
  5%  ts_paint windows10-64-qr opt e10s stylo            360.33 -> 343.00
  5%  sessionrestore windows10-64-qr opt e10s stylo      358.17 -> 341.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=17694