1618925 - 2.98 - 3.77% displaylist_mutate (linux64-shippable-qr) regression on push 0d3fcf6f3aaa071117de02f1350876d1203a1069 (Thu February 27 2020)

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

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

Talos has detected a Firefox performance regression from push:

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

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

Regressions:

4% displaylist_mutate linux64-shippable-qr opt e10s stylo 2,887.78 -> 2,996.76
3% displaylist_mutate linux64-shippable-qr opt e10s stylo 2,894.01 -> 2,980.29

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

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

[Hiroyuki Ikezoe (:hiro)]

The displaylist_mutate didn't regress in my run (bug 1510030 comment 7), but it makes sense. In the run I actually saw some regressions happened only on shippable builds.

Anyway, what is the actual threshold number (percentage?) we allow regressions?

I actually prepared additional patches to improve display item buildings a bit in case this display_mutate was regressed by my change as Glenn pointed out in a review comment. But I can't promise the patches reduce the number to zero (probably it doesn't).

FWIW, I just pushed a try with those patches;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4db0ccab373a39d160a1a72fb4abb30c6bdf0873

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I did push another try which is doing a bit more efficient way.

The result is still worse.

On the Linux 64 opt build, a subtest flattened_mutate.html got improved (4.61%), other subtests were not changed.

On the Linux 64 shippable build, the same subtest, flattened_mutable.html also got improved (3.93%), but mutate.html were regressed (4.07%).

As you may notice, the scores of the mutate.html on shippable (around 3400) are originally worse than the scores (around 3800) on normal opt builds. So, I think this extreme test case is not suitable for PGOs, and I am pretty sure such kind of extreme test cases are not used in PGO builds.

Glenn, what do you think about this result?

What I really don't understand is the patches used in the result should improve performance regardless of whether it's PGO or not.

Hmm maybe the score on the shippable build is not worse, it's "3,865.63 ± 0.87" in the try result and it's "3,831.73 ± 0.93" in the result on autoland when bug 1510030 landed.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1618925 - Bail out from AddAnimationsForDisplayItem if the corresponding EffectSet is empty. r?boris

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1618925 - Bail out from AddAnimationsForWebRender if there is no animation on the target frame. r?kats!,boris!

Calling CreateOrRecyleWebRenderUserData is a bit expensive in the case where
there is no animation. The case is pretty common.

Note that even if there is a WebRenderAnimationData having an AnimationInfo,
on the nsIFrame, it's going to be discarded since the WebRenderAnimationData
is not marked as being used.

Depends on D64852

Comment 5

[Hiroyuki Ikezoe (:hiro)]

I pushed the patches to review. Let's see the results when it lands.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fd14bf88cbd0
Bail out from AddAnimationsForDisplayItem if the corresponding EffectSet is empty. r=boris
https://hg.mozilla.org/integration/autoland/rev/1108b2535139
Bail out from AddAnimationsForWebRender if there is no animation on the target frame. r=kats,boris

Comment 7

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

The displaylist_mutate didn't regress in my run (bug 1510030 comment 7), but it makes sense. In the run I actually saw some regressions happened only on shippable builds.

Anyway, what is the actual threshold number (percentage?) we allow regressions?

I actually prepared additional patches to improve display item buildings a bit in case this display_mutate was regressed by my change as Glenn pointed out in a review comment. But I can't promise the patches reduce the number to zero (probably it doesn't).

FWIW, I just pushed a try with those patches;
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4db0ccab373a39d160a1a72fb4abb30c6bdf0873

Depending on the framework of the regression, in this case 2%.

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/fd14bf88cbd0
https://hg.mozilla.org/mozilla-central/rev/1108b2535139