972675 - Gaia Calendar scroll performance drastically affected if z-index is greater than zero

Status: RESOLVED WORKSFORME

(Core :: Layout, defect, P1)

Description

[Miller Medeiros [:millermedeiros]]

having a performance issue on the calendar app related to APZ + z-index. Adding z-index greater than zero to an element is reducing the scroll FPS by around 50%.

steps to reproduce:
1) update gaia to latest (1.3 or master)
2) run `make reference-workload-medium` to populate calendar with events (or create a couple events manually)
3) revert commit https://github.com/mozilla-b2g/gaia/commit/44174c8032b77d304f7abf933b4d5d527baa98af (or add any z-index greater than 0 to these elements)
4) open calendar app
5) compare scroll performance of "day view" scroll on a day that contains events with a day without events.
6) close the calendar app
7) disable APZ
8) restart calendar app and test scroll performance again

the z-index was removed because of Bug 942854 (I was not able to reproduce the glitches on the Hamachi). I need to re-enable the z-index to be able to fix Bug 972666

Comment 1

[Miller Medeiros [:millermedeiros]]

I got an email from Mason Chang telling that he could help to investigate the problem, but he is travelling today, so not sure when/if he will be able to do it anytime soon.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Ahh interesting, scrolling was really bad when I did this:

https://bugzilla.mozilla.org/show_bug.cgi?id=968478

Try that and see if it helps. I remember it drastically helping FPS when I tested.

Comment 3

[Miller Medeiros [:millermedeiros]]

background color did not solve the slow down (not sure if it affected anything). I also tried to add another wrapper around all the `.hour` elements and set position to relative/absolute and set a z-index and background-color to that element as well, but still same perf results.

The only things that I tried that improves perf, causes the lines overlap (Bug 972666) - adding a z-index to the `.hour` elements or removing the the z-index from the `.event`...

My best bet on how to solve this problem is to avoid z-index altogether and change the whole calendar structure; but this would probably take many days since it would break many unit tests and would probably introduce other bugs during the process.. so I don't recommend doing that (it is not a trivial change by any means)

Comment 4

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

I haven't tried anything here yet but based on previous discussions with tn I suspect this might be happening because we're sandwiching non-scrollable content between two pieces of scrollable content in terms of the z-index. As a result we cannot layerize the scrollable content properly and this results in the bad scrolling perf.

Comment 5

[Mason Chang [Inactive] [:mchang]]

I'll take a look once some 1.4 blockers die down.

Comment 6

[Mason Chang [Inactive] [:mchang]]

I just found a scrolling regression w/ the SMS app in bug 974081 in Gecko. It played with the z-index of chrome. Can you try with the patch in that bug on mozilla-central and your fixes to see if you still have the scroll FPS issue? Thanks!

Comment 7

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

This might also be a result of bug 965593.

Comment 8

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

Oh sorry, that's exactly what Mason was suggesting in comment 6.

Comment 9

[Jason Smith [:jsmith]]

Blocks a blocker.

Comment 10

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

It's a bit confusing to have this bug marked as a 1.3+ blocker when it's not actually happening currently on 1.3 code. It would only happen if we were to back out the z-index change in bug 942854. Nevertheless, the patch I just put on bug 972666 shows one possible way forward - we can back out the z-index change and also insert a new stacking context in the right spot to prevent the scrolling performance regression that this bug is about.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #10)
> It's a bit confusing to have this bug marked as a 1.3+ blocker when it's not
> actually happening currently on 1.3 code. It would only happen if we were to
> back out the z-index change in bug 942854. Nevertheless, the patch I just
> put on bug 972666 shows one possible way forward - we can back out the
> z-index change and also insert a new stacking context in the right spot to
> prevent the scrolling performance regression that this bug is about.

See https://bugzilla.mozilla.org/show_bug.cgi?id=972666#c7.

Comment 12

[Jason Smith [:jsmith]]

Discussed in bug 965593 - we're no longer pursuing the real solution here. Unblocking this for 1.3.

Comment 13

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

Bug 965593 has been backed out, so putting a high z-index of stuff shouldn't be a problem any more.

Comment 14

[Timothy Nikkel (:tnikkel)]

I took a look at the display lists involved here. I don't think bug 965593 should have any effect either way. Without backing out https://github.com/mozilla-b2g/gaia/commit/44174c8032b77d304f7abf933b4d5d527baa98af the scrollbars are above the scrollable content, and that should be the same either with or without bug 965593.

When I backout https://github.com/mozilla-b2g/gaia/commit/44174c8032b77d304f7abf933b4d5d527baa98af then I do see the problem like comment 0 describes. In this case there is some non-scrollable content as well as scrollbars sandwiched between scrollable content both above and below it. So moving the scrollbars would not fix this as we would need to move the non-scrollable content as well.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

Without this we don't hit APZ performance target for 1.4.

Comment 16

[Miller Medeiros [:millermedeiros]]

no longer block 972666 since we found a way to avoid the performance regression - added a "wrapper" element and set z-index:0 and position:relative (creates a new stacking context).

Comment 17

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Does drawing the scrollbars in the compositor would help here ? There is a wip at https://bug814435.bugzilla.mozilla.org/attachment.cgi?id=8341328

This issue has been seen on sms, the call log and the wifi panel of settings. It's likely lurking into some other parts of the Gaia codebase as well.

Comment 18

[Nicolas Silva [:nical]]

This is also affecting the network list in the settings app, as well as sms and call-log (but not contacts)

Comment 19

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

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #17)
> This issue has been seen on sms, the call log and the wifi panel of
> settings. It's likely lurking into some other parts of the Gaia codebase as
> well.

(In reply to Nicolas Silva [:nical] from comment #18)
> This is also affecting the network list in the settings app, as well as sms
> and call-log (but not contacts)

Are these issues currently happening on Gaia master? If so you should file separate bugs for them. This bug is tracking a regression that happens only after a code change because of some layerization problems in layout. If the issues you guys are talking about are also dependent on code changes it would help to provide patches that reproduce the problem, because they may not all have the same underlying cause.

Comment 20

[Mason Chang [Inactive] [:mchang]]

Miller, do you have a calendar patch that reproduces this bug? Then we can take a look at it. Thanks!

Comment 21

[Miller Medeiros [:millermedeiros]]

Attachment: patch to re-introduce bug

to create the patch I simply reverted these 2 commits:

  git revert 646b1110028229094de9e802f22c95f027184e70
  git revert 78dc0c0013d25de52175e750e4be46bfb2727ef3

which are patches for Bug 972666 and Bug 942854

Comment 22

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Trees While Scrolling

In the version with the bad scrolling, the APZ scrollable layer is marked as non-visible, which seems broken.

Comment 23

[Mason Chang [Inactive] [:mchang]]

Attachment: Paint Dump of Broken Build

Comment 24

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8388018 [details]
Paint Dump of Broken Build

The problem is obvious from here. In the before optimization dump there are ScrollLayer items, then the OwnLayer for scrollbars, then some other content, and then ScrollLayer items. In order for async scrolling to work correctly all ScrollLayer items for the same scrollframe need to be consecutively with no other content inbetween them (except for scrollbar items, those are automatically moved up and out from inbetween ScrollLayer items). This is an unfortunate limitation of the current way async scroll layers are implemented that we hope to remove in the future. But for now we'll have to work around it. To work around it adjust the page so that the z-index of the content that is inbetween the ScrollLayer items is either below or above all ScrollLayer items.

Comment 25

[Mason Chang [Inactive] [:mchang]]

Just chatted with Miller. Since there is a workaround in the Calendar app, this issue no longer exists for 1.4, we can close this bug. I filed bug 981708 to fix the platform issues stated in Comment 24.

Comment 26

[Milan Sreckovic [:milan] (needinfo for best results)]

Even with this being resolved right now, tagging as 1.5 instead to make sure any regressions get in the right bucket.