974081 - Scrolling w/ the SMS App is Choppy and Checkerboards

Status: RESOLVED DUPLICATE of bug 975221

(Firefox OS Graveyard :: Gaia::SMS, defect, P1)

Description

[Mason Chang [Inactive] [:mchang]]

Scrolling with a make reference-workload-light on a hamachi is really slow and choppy with the light reference workload. Find and fix the problems.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: Profile of Messages while Scrolling

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree while Scrolling w/ bug 965593

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: messages_painting.mov

Messages overpainting while scrolling.

Comment 4

[Benoit Girard (:BenWa)]

Can you play with the inspect and see if you can narrow down what's responsible for the over invalidation? That would certainly explain why some random Rasterize boxes are large.

Comment 5

[Julien Wajsberg [:julienw]]

I think this happens also in the Call log.

This regressed about 1 week ago, it could make sense to try a bisect.

Comment 6

[Mason Chang [Inactive] [:mchang]]

Just tried with commit 28f9dd73bccfafb62b6f883296ed42f2518e0fd9 from bug 967878, putting a scrollable background. That didn't seem to help.

Comment 7

[Julien Wajsberg [:julienw]]

I think this is a regression in Gecko, because this worked fine back when I merged bug 967878.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Yup, just tried with gaia master and gecko 166464:c78aa51b8a9c from feb 1st and scrolling is a lot smoother. We checkerboard, but it's much smoother. Bissecting now.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Found this while bisecting:

The first bad revision is:
changeset:   167385:477e7d2eb1d7
user:        Timothy Nikkel <[e-mail]>
date:        Thu Feb 06 16:46:21 2014 -0600
summary:     Bug 965593. Only use large z-index on root scroll frames to make overlay scrollbars draw above other content. r=roc

Comment 10

[Mason Chang [Inactive] [:mchang]]

Just tested by backing out 167385:477e7d2eb1d7, scrolling feels a lot smoother. The scroll bar is kind of jumpy, but overall feels better.

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: Backout Bug 965593

Backout bug 965593 due to SMS scrolling regression.

Comment 12

[Timothy Nikkel (:tnikkel)]

What about bug 965593 makes us paint more? Bug 965593 is a 1.3 blocker so we can't simply backout, we'll need to consider our options how to fix bug 965593 differently or how to fix this bug differently, or which bad bug we want to live with.

Comment 13

[Timothy Nikkel (:tnikkel)]

Looking at the layer dump it looks like we get a container layer that is not visible but has populated frame metrics. This implies it is a scroll info layer, which probably means we failed to merge all of the scroll layer items that contained the scrolled content.

Comment 14

[Timothy Nikkel (:tnikkel)]

It's the background added in bug 967878 that goes below the scrollbars that causes the scrollable layer creation to fail.

My inclination would be to back out bug 965593, and then figure out why the scrolling experience is so bad when we fail to create a scrollable layer in bug 965593. This bug shows that the scrolling experience isn't completely horrible when we fail to build a scrollable layer, so there must be something else making bug 965593 worse.

Comment 15

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

(In reply to Timothy Nikkel (:tn) from comment #14)
> This bug shows that the scrolling experience isn't completely
> horrible when we fail to build a scrollable layer, so there must be
> something else making bug 965593 worse.

It might be that at the time we were experiencing bug 965593, we hadn't reduced the paint interval to 40ms and so it looked a lot worse when it fell back to sync scrolling. Bug 965593 will probably be less severe now if the patch for it were to be backed out.

I'm sort of on the fence about this. As I said before I like that we're consistent with the z-index between B2G and OS X desktop but on the other hand maybe we need to fix the layerization stuff properly first. So yeah, if you think it's better to back out bug 965593 then I'm ok with it.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Does bug 965593 need backing out from v1.3 as well?

Comment 17

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/o bug 965593

This layer tree is very interesting, and I didn't notice it the first time around. We gralloc a lot fewer GrallocTextureHosts w/ bug 965593, and if I'm reading this right, we're only single buffering. Without the patch, we have double buffered thebes layers everywhere (which is what I'm more used to seeing). Could that explain the stuttering?

Comment 18

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

(In reply to Timothy Nikkel (:tn) from comment #14)
> It's the background added in bug 967878 that goes below the scrollbars that
> causes the scrollable layer creation to fail.
> 
> My inclination would be to back out bug 965593, and then figure out why the
> scrolling experience is so bad when we fail to create a scrollable layer in
> bug 965593. This bug shows that the scrolling experience isn't completely
> horrible when we fail to build a scrollable layer, so there must be
> something else making bug 965593 worse.

FYI I cooked up a WIP on bug 972666 which solves this problem by inserting a div just inside the overflow:scroll container. The div contains all the scrollable content but also has position:relative;z-index:0 set to force a new stacking context. This has the net effect that all the content inside the overflow:scroll container that uses z-index sits in its own stacking context under the scrollbar, and so there's less chance of sandwiching the scrollbar.

Comment 19

[Mason Chang [Inactive] [:mchang]]

After talking with :kats and :tn, we decided to back out bug 965593 everywhere. A fix to 965593 is too risky for 1.3, so make bug 965593 a 1.4 blocker instead.

Comment 20

[Mason Chang [Inactive] [:mchang]]

Ryan, please backout bug 965593 everywhere. Thanks!

Comment 21

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

I can do the backout once tn has posted his summary and we get confirmation from release drivers.

Comment 22

[Jason Smith [:jsmith]]

Clearing needinfo - we are not backing this out.

Comment 23

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

Luckily for us our partners noticed this regression too. Tapas filed the same bug and managed to convince Jason that bug 965593 needs to be backed out. Duping this one.