Closed Bug 846901 (Email-Scroll) Opened 7 years ago Closed 7 years ago

Email scroll performance is very poor (<50fps) and stuttery

Categories

(Core :: Layout, defect, blocker)

ARM
Gonk (Firefox OS)
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla22
blocking-b2g tef+
Tracking Status
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: godavari, Assigned: roc)

References

Details

(Whiteboard: ux-tracking)

Attachments

(5 files, 1 obsolete file)

Email scroll performance is still poor.
Evaluating patches from https://bugzilla.mozilla.org/show_bug.cgi?id=840431
Whiteboard: [caf-v1.0.1]
Our current scroll is less than 50 fps and with the optimizations with the above patches, it might reach upto 50. But the competitor OSs are reaching upto 58 and we should really target for the same. I can provide the test email account used incase required but it should be reproducible with any account with at least 150 messages in the inbox.
Group: qualcomm-confidential
blocking-b2g: --- → tef+
Whiteboard: [caf-v1.0.1]
Severity: normal → blocker
Andrew/Kevin, WDYT about this one?  I don't see an open bug to which we can dupe this but there are at least 2 other open email scrolling performance bugs.
Flags: needinfo?
I know of no effort or bug to improve our FPS other than the already referenced family of bugs on ellipsis making the repaints slow.

The biggest problem we've had with scrolling, in my experience, is choppiness from synchronization interfering with scrolling.  This will be improved by Vivien's work on moving the back-end into a worker on bug 814257, but won't do anything for improving peak FPS, especially once the e-mail app has synchronized the folder and is otherwise effectively idle.
Flags: needinfo?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Do we think the ellipsis fix will improve the FPS enough?
I thought Cjones conclusion on ellipsis fix was that it had negligible effect.
What is 'enough'?  Comment 0 implies we want 58 fps, it sounded like cjones was targeting 50fps?  The patch I tried certainly didn't get us to 50fps.

Speaking from the experience of this past Gaia perf week in Paris, I don't think any of we Gaia developers really have any idea how to determine what is slowing down the painting process.  Which is to say, I think it's going to be hard for us to investigate, let alone fix such things.  I would guess we want to use some combination of:
- using gdb to trigger dumps of those display rectangle/paint hierarchies and try and figure out what is so costly
- other printf based probes in the layout/paint code
- using a real statistical profiler, but the talk I hear is that stack unwinding in general is still a serious problem for us on the devices, so it's not clear Oprofile/perf would do any better.  But maybe it's just our home-brew profiler that has trouble?
Is this fixed (or significantly improved) by bug 840431 landing?
Jeff, can you provide some assistance to the Gaia developers' performance measurements?
Flags: needinfo?(jmuizelaar)
Just a regular gecko profile should help us get a general idea what's going on.
Flags: needinfo?(jmuizelaar)
I'm working on this.

One thing that's hurting is that we're using a lot of nested overflow:hidden elements in the messages window. If you could limit the use of overflow:hidden to the minimal set of elements that really need it, that would help a bit.
(I'm working on improving Gecko internals to make those nested overflow:hiddens cheap in bug 841192, but that's not for the 18 branch.)

Also, we should have AsyncPanZoomController enabled for scrolling in apps, but that's also not for the 18 branch.

(In reply to Andrew Sutherland (:asuth) from comment #6)
> - using gdb to trigger dumps of those display rectangle/paint hierarchies
> and try and figure out what is so costly
> - other printf based probes in the layout/paint code
> - using a real statistical profiler, but the talk I hear is that stack
> unwinding in general is still a serious problem for us on the devices, so
> it's not clear Oprofile/perf would do any better.  But maybe it's just our
> home-brew profiler that has trouble?

I believe bug 779291 adds support for stack unwinding in SPS on B2G, and that is in the process of landing. But that is also not for the 18 branch.
With the patches in bug 840431 I hit 50fps or a little more some of the time when I'm scrolling. It's mostly in the 40s. The fps varies a lot depending on how far and fast I scroll, so I wonder exactly what we should be measuring.

Anyway, I'm trying to find ways to improve this.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> I'm working on this.
> 
> One thing that's hurting is that we're using a lot of nested overflow:hidden
> elements in the messages window. If you could limit the use of
> overflow:hidden to the minimal set of elements that really need it, that
> would help a bit.

That's very useful information, thanks!

On the messages in the list, the overflow:hidden's are on the message item (.msg-header-item) and on the 3 elippse-using fields (.msg-header-author, .msg-header-subject, .msg-header-snippet)  Can we drop the overflow: hidden if we are using text-overflow: ellipsis, or does that make little difference?  I believe we can drop the overflow on the .msg-header-item since it was a paranoia backstop against those fields overflowing.

In terms of the containers that hold it, we can probably change .msg-list-scrollouter from "overflow-x: hidden" to "overflow-x: visible" since it should be clipped by its owning card.

In terms of its ancestors, I think ".card" has to keep "overflow: hidden", but both #cards and #cardContainer are "overflow: hidden" and probably don't need to be since we size them both to be the same size as the viewport.

The CSS to edit is apps/email/style/message-cards.css for the .msg- prefixes and apps/email/style/mail.css for the others.
Setting back to tef? until we know what FPS we're targeting. roc notes that we're already in the 40s/50s in comment 12 after landing bug 840431, so we need an explicit target and explanation of criticality.

Also sending to roc, since comment 12 suggests he's investigating.
Assignee: nobody → roc
blocking-b2g: tef+ → tef?
Flags: needinfo?(suryanar)
(In reply to Andrew Sutherland (:asuth) from comment #13)
> On the messages in the list, the overflow:hidden's are on the message item
> (.msg-header-item) and on the 3 elippse-using fields (.msg-header-author,
> .msg-header-subject, .msg-header-snippet)  Can we drop the overflow: hidden
> if we are using text-overflow: ellipsis, or does that make little
> difference?

You can't remove it, text-overflow needs it.

> I believe we can drop the overflow on the .msg-header-item
> since it was a paranoia backstop against those fields overflowing.

Yes, I've removed that as an experiment and everything seems fine, but it doesn't help much on its own.

> In terms of the containers that hold it, we can probably change
> .msg-list-scrollouter from "overflow-x: hidden" to "overflow-x: visible"
> since it should be clipped by its owning card.

That won't help since it's overflow-y:auto.

> In terms of its ancestors, I think ".card" has to keep "overflow: hidden",
> but both #cards and #cardContainer are "overflow: hidden" and probably don't
> need to be since we size them both to be the same size as the viewport.

OK I'll try removing those.
Target scroll FPS is 58.
Current value is ~50fps.
Flags: needinfo?(suryanar)
(back to tef+ now that targets are declared here.  This is part of the parity with other platforms running on the same hardware requirement we have had all along)
blocking-b2g: tef? → tef+
Reverting the fix for bug 828266 helps significantly. The fix for that bug works by making cards to the left and right of the visible card barely visible; this is good for causing the contents of those cards to be prerendered, but bad for scrolling performance in the visible card. I'm trying to back that fix out and fix bug 828266 with a fix in Gecko instead.
58fps is meaningless if scrolling constantly freezes, stutters to -10fps, or takes +500ms to respond to user touch events. Per Andrew's comment #3, I've added a comment to bug 814257 in the hope that it will address these underlying issues, and we're tracking both bugs as ux-most-wanted (new UX whiteboard flag for highest priority issues)
Whiteboard: u=user c=Email s=ux-most-wanted
Alias: Email-Scroll
Correct. We also need the stuttering fixed. Work is in progress for that.
Attachment #725228 - Flags: review?(matt.woodrow) → review+
The E-Mail app's card containing the folder list for some reason is twice as wide as the screen, so I had to boost the allowable dimensions for prerendering in nsDisplayTransform::ShouldPrerenderTransformedContent.
Attachment #725231 - Flags: review?(matt.woodrow)
With the previous Gecko patch, the fix in bug 828266 is no longer necessary and we can remove it, buying about 5fps scrolling the message list on my device.
Attachment #725235 - Flags: review?(bugmail)
I think we should also uplift the patch from bug 815591 (and the fix for the bug it exposed, bug 846144) to b2g-18. They help only a tiny bit, but every bit helps...
Note, similar hacks made to the settings app (bug 827382) and the SMS app might not be needed after the Gecko fix in comment #22. In that case, removing those hacks will probably give a small speedup in those applications too.
With these patches I get peak scrolling FPS of around 56 or 57, although the FPS varies a lot as my finger moves. Next week I'll reprofile and see if we can squeeze out any more.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> The E-Mail app's card containing the folder list for some reason is twice as
> wide as the screen, so I had to boost the allowable dimensions for
> prerendering in nsDisplayTransform::ShouldPrerenderTransformedContent.

Actually the twice-as-wide thing could be a Gecko issue, I'm looking into it.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Actually the twice-as-wide thing could be a Gecko issue, I'm looking into it.

It's not.

It looks like .fld-accounts-container has translateX(-100%) which puts it to the left of the left edge of .card-folder-picker, and it's not being clipped by .folder-scoller-region even though it looks like it should be, from the markup. Maybe the app is rearranging the DOM.
Comment on attachment 725235 [details] [diff] [review]
Back out fix for bug 828266

r=asuth, but this obviously should not land before the platform fix.

Thanks very much for fixing the platform for this!  I am glad to see you are not resting on your laurels following your extremely well deserved award! ;)
Attachment #725235 - Flags: review?(bugmail) → review+
Comment on attachment 725236 [details] [diff] [review]
Reduce complexity of clipping in display list. landed: https://github.com/mozilla-b2g/gaia/pull/8681

r=asuth, Thanks!

I've landed this for you since there's no benefit for this to wait it out.

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/8681
https://github.com/mozilla-b2g/gaia/commit/76185af4659982e750bb625f78bd5c81346eefb0
Attachment #725236 - Attachment description: Reduce complexity of clipping in display list → Reduce complexity of clipping in display list. landed: https://github.com/mozilla-b2g/gaia/pull/8681
Attachment #725236 - Flags: review?(bugmail) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> It looks like .fld-accounts-container has translateX(-100%) which puts it to
> the left of the left edge of .card-folder-picker, and it's not being clipped
> by .folder-scoller-region even though it looks like it should be, from the
> markup. Maybe the app is rearranging the DOM.

Andrew, if you could look into this and fix it, that would let me reduce the risk of the patch in comment #22. Basically we don't want to have nested elements that both have translateX(-100%).
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #31)
> Andrew, if you could look into this and fix it, that would let me reduce the
> risk of the patch in comment #22. Basically we don't want to have nested
> elements that both have translateX(-100%).

The UX spec calls for an animated transition from our folder list to our account list and back again within that card (as opposed to just having another card to transition to, which is what we did previously).  If our use of translateX is temporary and only lasting as long as the transition, is that okay?  Or would it be better to just animate 'left' in that case and take the performance hit?  We could also go back to multiple cards or the like.
(In reply to Andrew Sutherland (:asuth) from comment #32)
> The UX spec calls for an animated transition from our folder list to our
> account list and back again within that card (as opposed to just having
> another card to transition to, which is what we did previously).  If our use
> of translateX is temporary and only lasting as long as the transition, is
> that okay?

Yes.

> Or would it be better to just animate 'left' in that case and
> take the performance hit?  We could also go back to multiple cards or the
> like.

If there's a way to get the effect you want while having the folder list and account list be siblings instead of nested, that sound best. It seems to me this should be quite doable while keeping the current visual behavior.

Adding overflow:hidden to .card-folder-picker also fixes the oversized card problem. However it causes a regression when you flip from the folder picker to the account picker and back. I'm not sure why. It seems to me things would be simpler and more robust if the folder list and account list were siblings.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #33)
> If there's a way to get the effect you want while having the folder list and
> account list be siblings instead of nested, that sound best. It seems to me
> this should be quite doable while keeping the current visual behavior.

i.e. I'm thinking you would have separate folder list and account list cards that are both translateX(-100%) when hidden. To transition from folder list to account list or back you'd just set the z-index on the new one to put it in front and transition it into view with translateX(0) and when that ends, set the other one to translateX(-100%).
Comment on attachment 725231 [details] [diff] [review]
Support prerendering elements with animated transforms that are offscreen but only by a small amount

Review of attachment 725231 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I'm still somewhat worried that we won't ever be able to get accurate enough heuristics to make pre-rendering generally useful.

This is the second case (that I'm aware of) where we've needed pre-rendering for performance, and we've had to make platform changes both times. I think it's safe to assume that third party apps will need this too, and they won't (easily) be able to make platform changes.

We also don't want to unnecessarily pre-render content, since it's a waste of time and memory.

::: layout/base/nsDisplayList.cpp
@@ +3762,5 @@
>  
>    nsSize refSize = aBuilder->RootReferenceFrame()->GetSize();
> +  // Only prerender if the transformed frame's size is <= twice the
> +  // reference frame size (~viewport) in each direction.
> +  refSize += nsSize(refSize.width, refSize.height);

Definitely would prefer if we can avoid this with a gaia change.
Attachment #725231 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #35)
> ::: layout/base/nsDisplayList.cpp
> @@ +3762,5 @@
> >  
> >    nsSize refSize = aBuilder->RootReferenceFrame()->GetSize();
> > +  // Only prerender if the transformed frame's size is <= twice the
> > +  // reference frame size (~viewport) in each direction.
> > +  refSize += nsSize(refSize.width, refSize.height);
> 
> Definitely would prefer if we can avoid this with a gaia change.

Assume a gaia change.  I'm hoping :jrburke can take a look at it on (West Coast) Monday since he's very familiar with the e-mail app's card implementation and I am doing worker stuff.
Comment on attachment 726453 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8709 (landed: comment 41)

Splits out the account picker to a different card with an "overlay" animation, to avoid the nested translateX usage from before. Notable code changes:

* There is an AccountPickerCard in folder-cards.js now, and the parts of the account picker that FolderPickerCard used to do are now in that separate card. index.html now has
a card template for account-picker too.
* zindex for overlaid card decks are now incremented by just 10, instad of 100, because the shared dialog class only has a z-index of 100, and with two overlay decks (one for AccountPicker and one for the account config) that placed it under the zindex of 200, hiding the confirmation dialog. With increments of 10 it is now 100 vs 20. This should be fine as the 100 increment was arbitrary before, and probably too much of a jump.
* AccountPicker card shows the account config gear, but not last sync: since it seemed to make less sense in a listing of accounts.
* Clicking on the "Inbox" header in the far upper right, when AccountPicker is shown, results in a "navigate two cards back" navigation. So, two navigations stacked back to back. The _afterTransitionAction changes in mail-common.js are related to this. While it looks kind of neat, it is new, and may be undesirable. If this is not a desirable behavior, it will require a more invasive change to card navigation to sort out, as it was designed more around adjacent card navigation.

Because of the UX changes mentioned above, it should have a UX review, but I am not sure how to specify that for gaia code.
Attachment #726453 - Flags: review?(bugmail)
(In reply to James Burke [:jrburke] from comment #38)
> * zindex for overlaid card decks are now incremented by just 10, instad of

For some reason I thought our #cardContainer had its own stacking context so we would be safe from the dialogs, but it doesn't have a z-index after all.  I'm a bit afraid of adding one now an perturbing roc's investigation results.  This sounds fine given our current set of legal cards.

> * AccountPicker card shows the account config gear, but not last sync: since
> it seemed to make less sense in a listing of accounts.

This is actually what the UX specs call for, so this is an improvement.

> * Clicking on the "Inbox" header in the far upper right, when AccountPicker
> is shown, results in a "navigate two cards back" navigation. So, two
> navigations stacked back to back. The _afterTransitionAction changes in
> mail-common.js are related to this. While it looks kind of neat, it is new,
> and may be undesirable. If this is not a desirable behavior, it will require
> a more invasive change to card navigation to sort out, as it was designed
> more around adjacent card navigation.
> 
> Because of the UX changes mentioned above, it should have a UX review, but I
> am not sure how to specify that for gaia code.

I'm copying Casey so he's aware, but that should probably just be a follow-up unless the fallout from below makes things easier.  The proposed VD transition specs, as I am aware of them are here:
https://www.dropbox.com/sh/ekzrrz7um4uj6y0/NVzqB1D4hx/03%20Transitions#/

According to those, we're doing the tray animation wrong right now.  We should be sliding to the right to reveal the tray beneath us (without it moving) and then sliding back over it (but not sliding the tray) to hide it.  (Calendar does this, although its animation is oddly delayed.)  No guidance is provided on transitions within the tray there, but I would expect that since the goal of the tray is to close over the folder list, there would be no need to animate the folder list.


I believe this fix will address bug 828489 which was a complaint about the state of the folder/account list being sticky.
(In reply to Andrew Sutherland (:asuth) from comment #39)
> According to those, we're doing the tray animation wrong right now.  We
> should be sliding to the right to reveal the tray beneath us (without it
> moving) and then sliding back over it (but not sliding the tray) to hide it.
> (Calendar does this, although its animation is oddly delayed.)  No guidance
> is provided on transitions within the tray there, but I would expect that
> since the goal of the tray is to close over the folder list, there would be
> no need to animate the folder list.

I updated the transition so that the folder list now appears to be under the message list.

I also worked through some other changes suggested in IRC with :asuth. I squashed commits with an r=asuth from IRC.

Casey, I also made a short video of how it looks now so you can confirm, but it sounds like with the rest of the changes this brings it overall closer with original UX goals:
http://www.youtube.com/watch?v=KhN83KPSmoI
Comment on attachment 726453 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8709 (landed: comment 41)

landed on gaia/master:
https://github.com/mozilla-b2g/gaia/pull/8709
https://github.com/mozilla-b2g/gaia/commit/3aa6d4023a246cac1f23f088de40d641505f3f0d
Attachment #726453 - Attachment description: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8709 → Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8709 (landed: comment 41)
Attachment #726453 - Flags: review?(bugmail) → review+
This is the patch r+ed by Matt, but without increasing the allowed size of a prerendered layer. I verified that the latest Gaia commits by Andrew and James (thanks!!!) mean we don't need to increase the size anymore.
Attachment #725231 - Attachment is obsolete: true
Attachment #726557 - Flags: review+
Component: Gaia::E-Mail → Layout
Product: Boot2Gecko → Core
Comment on attachment 726557 [details] [diff] [review]
Support prerendering elements with animated transforms that are offscreen but only by a small amount, v2

This can land on inbound and mozilla-b2g18.
Attachment #726557 - Flags: checkin?
Comment on attachment 725228 [details] [diff] [review]
Skip nsDisplayWrapList construction if there's only one item to wrap and it already has the right frame

This can land in inbound and mozilla-b2g18.
Attachment #725228 - Flags: checkin?
/* Override animation for card list, should appear
 * to just be under the message list */
.card.before.card-folder-picker {
  transform: translateX(0);
}

What is this newly added style rule for? It's causing a slowdown because the folder picker is in the viewport :-(.
I experimented with some more optimizations, which --- together with these patches, removing the problematic style rule mentioned in comment #45, and the patch in bug 815591 (for which I have requested B2G18 approval) --- eke out a few more FPS. On my dev phone, in a Sent Mail folder with 100-ish messages, I get peak fps of 57-58 and sometimes 59. I'll call that victory until someone says otherwise :-).

I will put those extra optimizations in newly-filed bug 852489 to keep the scope of this bug reasonable.
Depends on: 852489
Attachment #725228 - Flags: checkin? → checkin+
Attachment #726557 - Flags: checkin? → checkin+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #45)
> /* Override animation for card list, should appear
>  * to just be under the message list */
> .card.before.card-folder-picker {
>   transform: translateX(0);
> }
> 
> What is this newly added style rule for? It's causing a slowdown because the
> folder picker is in the viewport :-(.

Scope creep on my requests for the patch; the actual transition specs call for the folder picker to be stationary and revealed by the motion of the folder list.  I was remiss in considering the implications of the change.  To do it correctly, it seems like we either need to set/clear "visibility: hidden" or position the card outside the viewport while it's not intended to be in use.  That seems like it will trigger pre-rendering issues.

So I'll file a spin-off bug that just removes that rule that blocks this bug and land it.  I'll also comment on bug 834665 which is about the tray transitions to make sure that when we do fix that we do the proper legwork and ensure there are no FPS regressions.
(In reply to Andrew Sutherland (:asuth) from comment #49)
> So I'll file a spin-off bug that just removes that rule that blocks this bug
> and land it.  I'll also comment on bug 834665 which is about the tray
> transitions to make sure that when we do fix that we do the proper legwork
> and ensure there are no FPS regressions.

Bug 852550 spun off and landed.  Bug 834655 commented on.
Hey guys, transition looks good to me.   Victoria should probably verify that it's as per transition spec.
Flags: needinfo?(vpg)
(In reply to Casey Yee [:cyee] from comment #51)
> Hey guys, transition looks good to me.   Victoria should probably verify
> that it's as per transition spec.

Because of the back-out landed as bug 852550 the video is now moot and we're definitely not meeting transition spec.  Please discuss any issues related to tray transitions on bug 834665.  (Note that I mis-typed the bug# in comment 50.)
Flags: needinfo?(vpg)
The gaia part landed to v1-train as part of the Big Bug 851124 uplift.
(but not to v1.0.1).
https://hg.mozilla.org/mozilla-central/rev/d94b25fadb3f
https://hg.mozilla.org/mozilla-central/rev/4d0d582c49ec
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Andrew, I think a gaia part should be uplifted here too, right ?
Flags: needinfo?(bugmail)
(In reply to Julien Wajsberg [:julienw] from comment #58)
> Andrew, I think a gaia part should be uplifted here too, right ?

These 3 landings all want to be uplifted:
https://bugzilla.mozilla.org/show_bug.cgi?id=846901#c30
https://bugzilla.mozilla.org/show_bug.cgi?id=846901#c41
https://bugzilla.mozilla.org/show_bug.cgi?id=852550#c1
Flags: needinfo?(bugmail)
John, I let you do that. If there are conflicts, Andrew or I will resolve them.
Flags: needinfo?(jhford)
v1.0.1: 577d13088ebdbd353d13910d3317e713a140415b (Bug 852550)
v1.0.1: b9cca452d985aa9da81b3bfb980446d2f4575a55
v1.0.1: dd9293fa40b49537ce08e3e3f30b686470c07414
Flags: needinfo?(jhford)
just to not forget: v1-train wants them too ;)
Flags: needinfo?(jhford)
Oh no, sorry, Comment 55 says I already did that.
Flags: needinfo?(jhford)
Whiteboard: u=user c=Email s=ux-most-wanted → ux-tracking
You need to log in before you can comment on or make changes to this bug.