1033468 - Home Screen Scrolling Performance when using home button to get to the top

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Homescreen, defect, P2)

Description

[Faramarz Rashed]

Attachment: home-screen-scrolling

scrolling through the home screen is not smooth.

STR:
1) make sure you have enough icon on the homescreen
   to scroll for more than a page.
2) scroll to the bottom of the screen.
3) press the "home" button.

Expected:
smooth scrolling to the top showing the search-bar at the top.

Actual:
janky behavior. check attached video.

Comment 1

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

I can't see anything in the video.

Comment 2

[[:fabrice] Fabrice Desré]

It's a bit slower and jittery when reaching the top. Quite noticeable in real life on my phone that we used for the video.

Comment 3

[Benoit Girard (:BenWa)]

The video doesn't render on the mac quicktime for me either.

Regardless I've seen this on my device. I suspect we don't have a JS API to async scroll. I heard some chatter during work weeks about getting this API in place. Kats?

Comment 4

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

Yeah the scroll-to-top feature is using JS-driven sync scrolling which is why it is janky. CC'ing kgrandon.

Comment 5

[Benoit Girard (:BenWa)]

Looks like bug 964097 is the optimial solution. However a scrollTo shouldn't require a style or layout flush and should require very little re-rasterization.

Why is this so slow? A quick profile would tell us what the bottleneck is. We should be able to sync scroll the homescreen at 60 FPS.

Comment 6

[Benoit Girard (:BenWa)]

Here's a video profile:
http://people.mozilla.org/~bgirard/cleopatra/#report=bb870de48e84e95b868425f51dc60f2407dca69e

Looks like we're bottlenecking on DisplayList related code taking a fixed 20ms per transaction.

Matt any way we can speed this up?

Comment 7

[Kevin Grandon :kgrandon]

The current scroll to top code was just a quick prototype, and I'm sure we could be doing something much more ideal. Even with perfect platform performance I'm sure we would see some jankiness just because we just scroll in chunks to the top of the screen with the current logic. Maybe we can do a better job here and try copying some APZ physics for 2.0. 

https://github.com/mozilla-b2g/gaia/blob/master/apps/verticalhome/js/app.js#L181

Comment 8

[Benoit Girard (:BenWa)]

Doing that means that the handoff between JS scrolling and APZ is going to be quite jarring even if we get the physics right.

Comment 9

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Girard (:BenWa) from comment #6)
> Here's a video profile:
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=bb870de48e84e95b868425f51dc60f2407dca69e
> 
> Looks like we're bottlenecking on DisplayList related code taking a fixed
> 20ms per transaction.
> 
> Matt any way we can speed this up?

It looks like we only have samples from after we stopped scrolling, assuming the video sync is correct.

We're definitely spending a huge amount of time in displaylist and layer builder, but it's hard to tell exactly why without full stack traces and the display list dumps.

Nothing really stands out in the symbols we do have, so I suspect the problem is that we have a lot of display items rather than a specific performance cliff.

One problem is that we build display items for everything within the display port, not what is visible on the screen. This is a huge amount of extra work, that is usually offset by being able to async scroll the results. We also align our display port sizes to tile boundaries, so most scrolls won't actually require any painting and we're building an enormous display list and only adjusting the scroll position on one layer.

One options would be to disable (or downsize) the display port if we detect that we're scrolling synchronously. This should speed up the scroll considerably, though we'd still have a slow paint when we transition back to display port mode. Would also suck if the user tries to async scroll at the same time, but maybe that's always going to be a bad time.

The other option is to detect that nothing has changed except the scroll position (and we're still aligned to the same tile bounaries), and do an empty transaction (skipping display list building). We'd need something similar to APZ on the main thread to update our layer transforms for the new scroll position. This would be fast for most paints, but when we cross a tile boundary we'd still need a full transaction and it could be janky.

Comment 10

[Benoit Girard (:BenWa)]

The video sync is not correct but you can still use the video as a indication of what was being profiled. The extra composite at the end is the scrollbar fading out + the status bar changing colors.

Comment 11

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

Kevin, should we have a ux-b2g flag on this one?

Doesn't sound like we can deal with this for 2.0; with the smooth scroll work, 2.1 is a possibility.  If this becomes a blocking issue, we should get rid of the "press home button to get to the stop" functionality in 2.0.

Comment 12

[Kevin Grandon :kgrandon]

(In reply to Milan Sreckovic [:milan] from comment #11)
> Kevin, should we have a ux-b2g flag on this one?
> 
> Doesn't sound like we can deal with this for 2.0; with the smooth scroll
> work, 2.1 is a possibility.  If this becomes a blocking issue, we should get
> rid of the "press home button to get to the stop" functionality in 2.0.

For 2.0 I think our options are limited. We can fix this in 2.1.

Adding a ni? on UX to see if they are happy with the current "scroll to top" behavior of the home button. If we're not we could either remove it, or perform an instant jump to the top.

Comment 13

[Jason Smith [:jsmith]]

Adding QA Wanted to get an updated video of the bug here.

Comment 14

[Stephany Wilkes]

Flagging Gordon to see if current performance is acceptable. What I saw during bug bash yesterday was, I thought, acceptable and would not lead me to block, but I want to check with the scroll expert. :)

Comment 15

[Ting-Yu Chou [:ting] (away)]

Not so sure, but it seems not janky if change to 4 icons a row.

Comment 16

[Pi Wei Cheng [:piwei] (inactive)]

(In reply to Jason Smith [:jsmith] from comment #13)
> Adding QA Wanted to get an updated video of the bug here.

Not sure if this is reproducing the bug but here's a video following STR on Flame 2.0 Aurora.

http://youtu.be/4kr8sUsIH-E

It is an engineering build so there are more icons on Homescreen to begin with, but I also installed 20 games from marketplace to add more things on Homescreen.

Tested on:
Device: Flame (512MB mem)
Build ID: 20140721082721
Gaia: b9d19011123487009c80d1200937652d58c434a0
Gecko: d69cd84b6824
Version: 32.0a2 (2.0)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 17

[Gordon Brander :gordonb]

(In reply to Benoit Girard (:BenWa) from comment #5)
> Looks like bug 964097 is the optimial solution. However a scrollTo shouldn't
> require a style or layout flush and should require very little
> re-rasterization.
> 
> Why is this so slow? A quick profile would tell us what the bottleneck is.
> We should be able to sync scroll the homescreen at 60 FPS.

Agreed: bug 964097 is the "right" way to fix this.

(In reply to Stephany Wilkes from comment #14)
> Flagging Gordon to see if current performance is acceptable. What I saw
> during bug bash yesterday was, I thought, acceptable and would not lead me
> to block, but I want to check with the scroll expert. :)

Agreed with swilkes. The current behavior is not smooth, but is not a blocker.

Comment 18

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8486561 [details] [review]
Github pull request

Hey Kip - this is a pull request for gaia in an attempt to use smooth scrolling behavior in the home screen. The desired functionality is that it should smoothly scroll the screen to the top of the screen when you press the home button and are scrolled down the home screen.

Unfortunately this seems to not work for me. I was wondering if you could take a look at this and let me know if we're doing something silly here. Thanks!

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8486561 [details] [review]
Github pull request

(In reply to Kevin Grandon :kgrandon from comment #19)
> Comment on attachment 8486561 [details] [review]
> Github pull request
> 
> Hey Kip - this is a pull request for gaia in an attempt to use smooth
> scrolling behavior in the home screen. The desired functionality is that it
> should smoothly scroll the screen to the top of the screen when you press
> the home button and are scrolled down the home screen.
> 
> Unfortunately this seems to not work for me. I was wondering if you could
> take a look at this and let me know if we're doing something silly here.
> Thanks!

Actually, it appears it might be a problem with the gaia build and preferences. Manually fiddling with the preferences on the device seemed to fix it, might just be a problem I have. This is working great, thanks!

Comment 21

[Kevin Grandon :kgrandon]

Comment on attachment 8486561 [details] [review]
Github pull request

Hey Chris - have a minute to review this? Let's finally use something better than our terrible manual scrolling!

Comment 22

[Chris Lord [:cwiiis]]

Comment on attachment 8486561 [details] [review]
Github pull request

I'd r+ this twice if I could. Oh wait, I can :)

Comment 23

[Chris Lord [:cwiiis]]

Unfortunately, the vertical home scrolling unit test is failing (I imagine a simple fix), and the Gbu preferences test is failing (but possibly unrelated?)

https://tbpl.mozilla.org/?rev=9e5678647ecd6ed6465dd9a646a815178f47dd70&tree=Gaia-Try

Comment 24

[Kevin Grandon :kgrandon]

(In reply to Chris Lord [:cwiiis] from comment #23)
> Unfortunately, the vertical home scrolling unit test is failing (I imagine a
> simple fix), and the Gbu preferences test is failing (but possibly
> unrelated?)
> 
> https://tbpl.mozilla.org/
> ?rev=9e5678647ecd6ed6465dd9a646a815178f47dd70&tree=Gaia-Try

Thanks, it looks like Gbu is probably related so I'll make sure it's fixed. The tests look pretty simple to update so I'll just streamline some changes into this patch. If there's anything significant I'll re-flag you if that's ok.

Comment 25

[Chris Lord [:cwiiis]]

(In reply to Kevin Grandon :kgrandon from comment #24)
> (In reply to Chris Lord [:cwiiis] from comment #23)
> > Unfortunately, the vertical home scrolling unit test is failing (I imagine a
> > simple fix), and the Gbu preferences test is failing (but possibly
> > unrelated?)
> > 
> > https://tbpl.mozilla.org/
> > ?rev=9e5678647ecd6ed6465dd9a646a815178f47dd70&tree=Gaia-Try
> 
> Thanks, it looks like Gbu is probably related so I'll make sure it's fixed.
> The tests look pretty simple to update so I'll just streamline some changes
> into this patch. If there's anything significant I'll re-flag you if that's
> ok.

yup, fine by me.

Comment 26

[Kevin Grandon :kgrandon]

Gbu and Gu are now green so going to land.

(In reply to Chris Lord [:cwiiis] from comment #22)
> Comment on attachment 8486561 [details] [review]
> Github pull request
> 
> I'd r+ this twice if I could. Oh wait, I can :)

Updated commit message to the new review status and landing :) 

https://github.com/mozilla-b2g/gaia/commit/4c99eadd4b416121fcc1d8bbc2a02c2bb9fad9b7

Comment 27

[Kevin Grandon :kgrandon]

Marking dependencies in case we decide to uplift this. Depends on both using the root frame for scrolling, and getting the scrollbar calculation in.

Comment 28

[:kip (Kearwood Gilbert)]

As per Comment 27, I have added a dependency to Bug 1022825, which is recommended to be uplifted together with this bug.

Comment 29

[:kip (Kearwood Gilbert)]

Attachment: Video - B2G Scrolling Comparison

I have performed a build with and without Bug 1033468 (Home Screen Scrolling Performance when using home button to get to the top) applied for comparison.  I have used a 120fps camera to compare the results in the attached video.

Please note that there are some compression artifacts necessary to make this video small; however, the difference is still visible.

Comment 30

[:kip (Kearwood Gilbert)]

(In reply to :kip (Kearwood Gilbert) from comment #29)
> Created attachment 8488943 [details]
> Video - B2G Scrolling Comparison
> 
> I have performed a build with and without Bug 1033468 (Home Screen Scrolling
> Performance when using home button to get to the top) applied for
> comparison.  I have used a 120fps camera to compare the results in the
> attached video.
> 
> Please note that there are some compression artifacts necessary to make this
> video small; however, the difference is still visible.
A 1080p, high bitrate version of this video can be downloaded here:

https://www.dropbox.com/s/y1y43wa5n5icq7b/b2g_js_vs_native_scrolling.mp4?dl=1

Comment 31

[Gregor Wagner [:gwagner]]

Kevin, can you triage the need for uplift here? It doesn't seen to be fixed in 2.1 and its a performance improvement.

Comment 32

[Josh Cheng [:josh]]

Triage:
Hi Vincent,
Kevin and Keven has approved to land this on 2.1.
Can you help to see whether the patch is able to land on 2.1? Thanks!

Comment 33

[Josh Cheng [:josh]]

Hi Kevin,
Could you raise patch approval for landing this patch on 2.1?
Thanks!

Comment 34

[Kevin Grandon :kgrandon]

We can't uplift as-is because I think we're missing some platform patches to allow us to do so. I think this might be bug 1022825, which we should get uplifted first.

I'll go ahead and request bocking status in that bug.

Comment 35

[Kevin Grandon :kgrandon]

Also adding a ni? on Kip to verify if we're just missing bug 1022825. I recall that something changed changed in the way that we were calling scrollTo(), I'm not sure what bug that is.

Comment 36

[:kip (Kearwood Gilbert)]

(In reply to Kevin Grandon :kgrandon [INACTIVE - heads down on Gij Issue] from comment #35)
> Also adding a ni? on Kip to verify if we're just missing bug 1022825. I
> recall that something changed changed in the way that we were calling
> scrollTo(), I'm not sure what bug that is.
The CSSOM-View W3 spec changed the DOM methods for smooth scrolling after Bug 1022825 had landed.  Bug 1045754 updated the DOM methods to match the updated spec.  If Bug 1045754 is landed, Bug 1088700 should also be applied to update GAIA's code for the home button smooth scrolling on B2G.

Comment 37

[Kevin Grandon :kgrandon]

Thanks for the info Kip!

Josh - if you really want this for 2.1, we would need to get bug 1022825, bug 1045754, and bug 1088700 all uplifted.

Because we shipped 2.0 like this, I'm not sure if it makes sense to block on this.

Comment 38

[Josh Cheng [:josh]]

Hi Kevin,
I think the fix is nontrivial and risky. 
Remove from 2.1. Thanks!