Closed Bug 972675 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Layout, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

()

RESOLVED WORKSFORME
1.4 S3 (14mar)
blocking-b2g 2.0+

People

(Reporter: mmedeiros, Assigned: mchang)

References

Details

(Keywords: perf, Whiteboard: [c=handeye p= s= u=1.4])

Attachments

(3 files)

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
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.
Blocks: 972666
Flags: needinfo?(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.
Flags: needinfo?(mchang) → needinfo?(mmedeiros)
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)
Flags: needinfo?(mmedeiros) → needinfo?(mchang)
Flags: needinfo?(mchang)
Keywords: perf
Priority: -- → P1
Whiteboard: [c=handeye p= s= u=]
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
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.
I'll take a look once some 1.4 blockers die down.
Flags: needinfo?(mchang)
Blocks: 974025
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!
Flags: needinfo?(mchang) → needinfo?(mmedeiros)
This might also be a result of bug 965593.
Blocks: 965593
Oh sorry, that's exactly what Mason was suggesting in comment 6.
Blocks a blocker.
blocking-b2g: --- → 1.3+
Blocks: gaia-apzc
Assignee: nobody → mchang
Status: NEW → ASSIGNED
Target Milestone: --- → 1.4 S3 (14mar)
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Discussed in bug 965593 - we're no longer pursuing the real solution here. Unblocking this for 1.3.
blocking-b2g: 1.3+ → ---
No longer blocks: 972666
Bug 965593 has been backed out, so putting a high z-index of stuff shouldn't be a problem any more.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
Flags: needinfo?(mmedeiros)
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.
No longer blocks: 965593
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Component: Panning and Zooming → Layout
Without this we don't hit APZ performance target for 1.4.
blocking-b2g: --- → 1.4+
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).
No longer blocks: 972666
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.
This is also affecting the network list in the settings app, as well as sms and call-log (but not contacts)
Whiteboard: [c=handeye p= s= u=] → [c=handeye p= s= u=1.4]
(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.
Miller, do you have a calendar patch that reproduces this bug? Then we can take a look at it. Thanks!
Flags: needinfo?(mmedeiros)
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
Flags: needinfo?(mmedeiros)
In the version with the bad scrolling, the APZ scrollable layer is marked as non-visible, which seems broken.
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.
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WORKSFORME
Even with this being resolved right now, tagging as 1.5 instead to make sure any regressions get in the right bucket.
blocking-b2g: 1.4+ → 1.5+
You need to log in before you can comment on or make changes to this bug.