1216924 - 9-13% Linux 64/Win7/WinXP tps regression on Mozilla-Inbound on October 20, 2015 from push 1e2fc068682e617ec5d72784cd2e34264157c88b

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Talos has detected a Firefox performance regression from your commit 1e2fc068682e617ec5d72784cd2e34264157c88b in bug 1216072.  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=1e2fc068682e617ec5d72784cd2e34264157c88b&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#tps

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 linux64,win32 -u none -t g2-e10s  # 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 tps --e10s

Making a decision:
As the patch author we need your feedback to help us handle this regression.
*** Please let us know your plans by Monday, or the offending patch will be backed out! ***

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

Comment 1

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

there are 4 regressions here and they are all e10s only:
13% win7 tps
9% winxp tps
10% linux64 tps
6% linux64 tp5o xres

perfherder isn't working right now, so we will need to rely on the alerts generated automatically and validating those with the graphs.

:benwa, this falls into our category of backing changes out as we have a 10%+ regression on windows tests, can you prioritize this and help us figure out the next actions on it?

Comment 2

[Benoit Girard (:BenWa)]

Thanks for flagging this down.

I think given that the patch also got us ~50% tp5_scroll and tscroll-asap improvements on windows and Linux we should keep this regression. This patch also align displayport behavior on Linux and Windows to behave more like mobile and Mac. The change in TPS make sense given that we round-out the display port so it will cause a small regression on pageload since we're building a displayport that doesn't benefit us on page load, but it will once we start scrolling.

We've been making good progress overall on improving the scores of the e10s+apz configuration and hopefully in the future we can target a patch at moving these scores forward.

Comment 3

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

that seems reasonable to me.  tps is a specific test where we load 50 pages at once, I suspect it is a more extreme case.  I do like the gains we get on scrolling- right now this is just making a call about what tradeoffs we want, much better scrolling, or a bit slower pageloading in bulk.  I don't see tp5o regressing, so a single pageload doesn't seem to be affected.

:vladan, thoughts?

Comment 4

[Vladan Djeric (:vladan)]

- comment 0 states that bug 1216072 is responsible, but I think that's the wrong bug #. Benoit landed patches for 3 bugs in push 1e2fc068682e617ec5d72784cd2e34264157c88b: bug 1216287, bug 1204136, and bug 1215596. Did it get bisected to a single bug?
- Which bug caused the improvements referenced in comment 2?
- If comment 2 is correct and the same bugs cause a 50% scroll APZ improvement and a 13% e10s tab-switch regression, the choice is not clear. Is E10S tabs-switching with APZ-disabled regressed?

Comment 5

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

it is bug 1204136 which caused this.  I am not sure what caused the improvements, I don't see alerts for those specifically related to this time window- I am collecting more numbers to look for the improvements.

Comment 6

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

there is a 4% win7 (bug 1208465) tps regression on uplift to aurora, but no linux64 or winxp.  This didn't seem to affect pgo/uplift.  Should we leave this open or close it?

Comment 7

[Vladan Djeric (:vladan)]

I spoke to Benoit, this patch is APZ-specific and the regressions and improvements should be APZ-specific as well. Since APZ is currently a Nightly-only feature, I'm ok with making trade-offs that only affect Firefox in the APZ configuration. This regression should be fixed before APZ rides the trains though (likely not soon).

We shouldn't see any regressions from this patch on Aurora. I guess the 4% win7 tps regression on Aurora 44 is caused by unrelated bug 1208465.

Let's leave this bug open as a blocker of meta-bug 1178298 (APZ riding the trains)

Comment 8

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

having it track firefox 45 then!

Comment 9

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

this is now on beta

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This stops aligning the displayport completely during displayport suppression, and fixes the tps regression.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=dd1abe874252&newProject=try&newRevision=ab72f4c809f4&framework=1

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 8729252 [details] [diff] [review]
Patch

Review of attachment 8729252 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. Thank you Kats!

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21884d000ede

Comment 13

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29f0eadfe982

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(See https://bugzilla.mozilla.org/show_bug.cgi?id=1254273#c9 for why this was backed out). I triggered the jobs that filed on my try push that I did for this patch, to see if it was this patch or the other one that caused the failure.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

It appears to be this patch. I'll investigate early next week.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Most likely there's some other test in the chunk that's causing the code to enter displayport suppression and not exit it. That seems bad, so we should definitely fix it.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

So displayport suppression is enabled while this test is running, and as far as I can tell there's no bug with that code. The suppression enables because we open a new tab and switch to it as part of the test, and the test runs fast enough that it finishes entirely before the tabswitcher code gets around to removing the displayport suppression. I tested a bunch of different scenarios as well as real browser usecases and the suppression seems to be working as intended. In particular, it's NOT the case that suppression gets left on accidentally, which is what I was worried about.

I filed bug 1255997 to fix an issue with the test where it wasn't accounting for the scrollbars properly. The displayport suppression exposed the issue, which was masked before by the displayport getting padded out to include the scrollbar area. With that patch in place the test seems to pass locally. I'll do a try push to confirm.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=afc1fe12b919

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe36b282024

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbe36b282024

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Marking 44/45 as unaffected as APZ is not enabled there. 46 will have APZ for a couple of betas but this is probably not worth uplifting there. We should uplift to 47 though, it should help with tab switch and resize performance a bit.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8729252 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: Tab switch operations a tiny bit slower because we paint a larger area.
[Describe test coverage new/current, TreeHerder]: talos coverage
[Risks and why]: low risk, small patch
[String/UUID change made/needed]: none

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Joel, Kats: Have we tested the fix in Nightly/try to confirm that the perf regression is fixed? I just want to make sure before taking this in 47. Thanks!

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Yup, perfherder shows the tps improvement:

https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-inbound,cbb19c1fe8eb2b93841a6623c0b095acce1535eb,0%5D&series=%5Bmozilla-inbound,ffbf04ed6819bacc30be90bb04e80df061763d89,1%5D&series=%5Bmozilla-inbound,f79913b4203541a74b7e06a4b8ecdbfaff379124,0%5D&series=%5Bmozilla-inbound,e0941ce949abc90f06b3fa7ef341a5e572892e4a,1%5D&series=%5Bmozilla-inbound,fa0057c2a95b75f216b32495ef140125506f7b35,0%5D&series=%5Bmozilla-inbound,2e5d3a39b4a8ec3cffbcf97002d6a4256457fe4d,1%5D&zoom=1457955390435.7542,1458004156406.4246,33.478247601053,129.13042151409647

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Here's the alert for it: https://treeherder.mozilla.org/perf.html#/alerts?id=439

Comment 26

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

thanks kats!

Comment 27

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8729252 [details] [diff] [review]
Patch

Thanks Kats, verified that this fix improves tab switching performance. Aurora47+

Comment 28

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bc7308b7f1db