Closed
Bug 1033468
Opened 10 years ago
Closed 10 years ago
Home Screen Scrolling Performance when using home button to get to the top
Categories
(Firefox OS Graveyard :: Gaia::Homescreen, defect, P2)
Tracking
(b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 fixed, b2g-master fixed)
RESOLVED
FIXED
2.1 S4 (12sep)
People
(Reporter: faramarz, Assigned: kgrandon)
References
Details
(Keywords: perf, Whiteboard: [systemsfe][c=progress p= s= u=])
Attachments
(3 files)
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•10 years ago
|
||
I can't see anything in the video.
Comment 2•10 years ago
|
||
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.
Updated•10 years ago
|
Updated•10 years ago
|
Summary: Home Screen Scrolling Performance → Home Screen Scrolling Performance when using home button to get to the top
Comment 3•10 years ago
|
||
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?
Flags: needinfo?(bugmail.mozilla)
Comment 4•10 years ago
|
||
Yeah the scroll-to-top feature is using JS-driven sync scrolling which is why it is janky. CC'ing kgrandon.
Flags: needinfo?(bugmail.mozilla)
Comment 5•10 years ago
|
||
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.
Depends on: 964097
Updated•10 years ago
|
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → All
Comment 6•10 years ago
|
||
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?
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 7•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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.
Flags: needinfo?(matt.woodrow)
Comment 10•10 years ago
|
||
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•10 years ago
|
||
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.
blocking-b2g: --- → backlog
Flags: needinfo?(kgrandon)
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Flags: needinfo?(kgrandon) → needinfo?(firefoxos-ux-bugzilla)
Comment 13•10 years ago
|
||
Adding QA Wanted to get an updated video of the bug here.
Component: Performance → Gaia::Homescreen
Keywords: qawanted
Comment 14•10 years ago
|
||
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. :)
Flags: needinfo?(firefoxos-ux-bugzilla) → needinfo?(gbrander)
Comment 15•10 years ago
|
||
Not so sure, but it seems not janky if change to 4 icons a row.
Comment 16•10 years ago
|
||
(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
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Comment 17•10 years ago
|
||
(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.
Flags: needinfo?(gbrander)
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][lead-review+]
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8486561 [details]
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!
Attachment #8486561 -
Flags: feedback?(kgilbert)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8486561 [details]
Github pull request
(In reply to Kevin Grandon :kgrandon from comment #19)
> Comment on attachment 8486561 [details]
> 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!
Attachment #8486561 -
Flags: feedback?(kgilbert)
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8486561 [details]
Github pull request
Hey Chris - have a minute to review this? Let's finally use something better than our terrible manual scrolling!
Attachment #8486561 -
Flags: review?(chrislord.net)
Comment 22•10 years ago
|
||
Comment on attachment 8486561 [details]
Github pull request
I'd r+ this twice if I could. Oh wait, I can :)
Attachment #8486561 -
Flags: review?(chrislord.net)
Attachment #8486561 -
Flags: review+
Comment 23•10 years ago
|
||
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
Assignee | ||
Comment 24•10 years ago
|
||
(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•10 years ago
|
||
(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.
Assignee | ||
Comment 26•10 years ago
|
||
Gbu and Gu are now green so going to land.
(In reply to Chris Lord [:cwiiis] from comment #22)
> Comment on attachment 8486561 [details]
> 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
Assignee: nobody → kgrandon
Status: NEW → RESOLVED
Closed: 10 years ago
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
Assignee | ||
Updated•10 years ago
|
Whiteboard: [c=progress p= s= u=] → [systemsfe][c=progress p= s= u=]
Assignee | ||
Comment 27•10 years ago
|
||
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•10 years ago
|
||
As per Comment 27, I have added a dependency to Bug 1022825, which is recommended to be uplifted together with this bug.
Comment 29•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
Kevin, can you triage the need for uplift here? It doesn't seen to be fixed in 2.1 and its a performance improvement.
Flags: needinfo?(khu)
Updated•10 years ago
|
status-b2g-v2.0M:
--- → wontfix
Updated•10 years ago
|
status-b2g-v2.1S:
--- → affected
Comment 32•10 years ago
|
||
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!
blocking-b2g: backlog → 2.1+
Flags: needinfo?(khu) → needinfo?(vliu)
Updated•10 years ago
|
status-b2g-master:
--- → fixed
Comment 33•10 years ago
|
||
Hi Kevin,
Could you raise patch approval for landing this patch on 2.1?
Thanks!
Flags: needinfo?(vliu) → needinfo?(kgrandon)
Assignee | ||
Comment 34•10 years ago
|
||
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.
Flags: needinfo?(kgrandon)
Assignee | ||
Comment 35•10 years ago
|
||
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.
Flags: needinfo?(kgilbert)
Comment 36•10 years ago
|
||
(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.
Flags: needinfo?(kgilbert)
Assignee | ||
Comment 37•10 years ago
|
||
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.
Flags: needinfo?(jocheng)
Comment 38•10 years ago
|
||
Hi Kevin,
I think the fix is nontrivial and risky.
Remove from 2.1. Thanks!
blocking-b2g: 2.1+ → ---
Flags: needinfo?(jocheng)
You need to log in
before you can comment on or make changes to this bug.
Description
•