943498 - email triggers 100+ ms sync reflows while scrolling

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While investigating bug 932418 we saw sync reflows of 100+ ms while scrolling the mail app:

  http://people.mozilla.org/~bgirard/cleopatra/#report=6d1a01f6f7b1bbbe79c5e30d474f524eb7278939

It appears this is largely being triggered by the getBoundingClientRect() calls here:

 https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/message_list.js#L747

We should investigate if we can remove or minimize these calls.  It may also be possible to reduce the reflow size by changing the DOM structure a bit.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I'm working this, but I don't see an immediately easy solution.  There are a lot of places in message_list.js that touch attributes that trigger sync reflows.  I've removed a few, but the problem moves to the next.

Here's a pull request of my work-in-progress if anyone wants to follow along:

  https://github.com/mozilla-b2g/gaia/pull/14090

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Logging some of our discussion on IRC for others, one way to reduce us poking at stuff is by not processing messages from the back-end while scrolling is in progress.  Specifically, message_list's onScroll could call MailAPI.pauseProcessing() when a scroll starts and set a 100ms timeout to call MailAPI.resumeProcessing() when scrolling is inferred to have ceased.  We reset the timeout every time we get a scroll event.  This could probably dovetail with our snippet requesting logic.

This would address the problem of snippet changes streaming in in succession quite neatly, although Mason's back-end changes for batching may also have a similar effect.

MailAPI.__bridgeReceive already does this somewhat because of our mozContact resolution logic.  This method could be slightly bulked up to also be triggered by the newly exposed pause/resume API.  See:
https://github.com/mozilla-b2g/gaia-email-libs-and-more/blob/master/data/lib/mailapi/mailapi.js#L2070

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Pull request at https://github.com/mozilla-b2g/gaia/pull/14276

This pull request does a few things:

1) Reduces how often we touch DOM attributes that can trigger sync reflows.
2) Reduces the message growth rate used to load messages as we scroll since we spend ~10ms rendering/reflowing for each message.
3) Dynamically increases the growth rate allowed if we are falling behind in our scrolling.  Without this I could sometimes "hit" the bottom or top of the screen before the next message loaded which ended the kinetic scroll suddenly.

Note, I found the visible scroll thumb useful for understanding what the app was doing here, but it might be nice to disable that for the user.  Its pretty much just a distraction that does not provide any useful information or functionality.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Here is a profile of scrolling with the candidate pull request:

  http://people.mozilla.org/~bgirard/cleopatra/#report=1ff70c7b852f5a350ae65dec810d20cee500c958

The majority of the sync reflows are now ~20ms.  Not perfect for our 60fps scroll target, but much improved.

The large 100ms reflow in the profile occurred when I saw network activity in the status bar.  Its possible a sync or some other operation slowed down processing forcing us to use a larger growth size there.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

On the pull request I proposed that we toss our existing onMessageSplice logic and replace it with something more understandable and that better handles multiple splices in succession.  I'm not sure we actually need to change onMessagesSplice to get the maximum win from your patch, so maybe we defer that, but here's what I'm thinking.

Things I assert:
- Message heights with inter-message margin padding are fixed.  (This was not originally known to be true when the code was first written.)
-- This value can easily be computed on demand and assumed to be available when needed.  If we don't have at least 2 messages in the DOM, then our calculations don't need the value.
-- We could add this height value to our "master cookie of evil"
- The height of the area at the top of the message list which is not messages but is scrollable (search box, potentially 'empty folder' banner) is largely un-changing; we can invalidate on demand.  I will call this 'topBoilerplateHeight'.
- The clientHeight of the scrollable area is largely unchanging; we can easily hook onresize to invalidate.
- The scroll splicing logic in all of its complexity is only concerned about the stability of the viewable area.  Changes happening outside the viewable area should not affect the viewable area. 
-- Changes below the viewable area do not need fixups; only changes above the viewable area.
- Given the current scrollTop and topBoilerplateHeight we can make all calculations required for scroll fix-ups without touching the DOM.

I think when Mason's patch revised batching patch lands, maximum correctness minimal risk would want us to latch scrollTop during the first splice, then write it back during the last splice.  This could be accomplished various ways.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

And when we do chnage onMessageSplice, I think we want a fairly small Gaia unit test.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8341226 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/14276

Qualitative experience on a ZTE Open device is that the patch greatly reduces incidences of hitching when scrolling doing retrievals from the database.

I'd like to discuss more what to do about the onMessageSplice thing.  I think you mentioned running without those changes and just doing the dynamic sizing.  Did that work out okay?  And/or do you think you can rationalize improving the onmessagesplice implementation?

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Sutherland (:asuth) from comment #7)
> I'd like to discuss more what to do about the onMessageSplice thing.  I
> think you mentioned running without those changes and just doing the dynamic
> sizing.  Did that work out okay?  And/or do you think you can rationalize
> improving the onmessagesplice implementation?

I wrote this on Github, but probably best to have the conversation here:

Well, I could just remove the changes to `onMessageSplice` if you think its going to change a lot in the near future. I think the majority of the improvements come from the reduced growth size and the change to call `onScroll` instead of `_onScroll` at the end of `onSliceRequestCompleted`.

Incidentally, I'm specifically trying to get smaller splices here in order to spread the work load. This is inherently less efficient from a throughput point of view, but is better from a latency (jank) point of view. It sounds at odds with the work "to batch up multiple splices", though. Am I not understanding?  Can you point me to Mason's patch?

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8341226 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/14276

I updated the pull request to remove the modifications to `onMessageSplice` and to address your other comments.  The scrolling still seems improved to me.

Here is a profile that shows the sync reflows are mostly around 20ms.  The few that are larger are during swipes where we hit the bug causing reflows during touch events.

  http://people.mozilla.org/~bgirard/cleopatra/#report=ddc944c62ec54140bf03d56a86e2cab5647b8cfe

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Hmm, just another datapoint:

I turned on paint flashing and it appears that the entire list is repainted when we append to it.  I see Layer::Mutated() in the profile which suggests perhaps the entire layer is being invalidated.

I'm not sure if there is anything we can do to avoid this, unfortunately.  We could try playing around more with the CSS overflow attribute and the DOM by creating additional sections within the list to contain messages added in a single splice.  Not sure if that would help, though.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8341226 [details] [review]
Pull request at https://github.com/mozilla-b2g/gaia/pull/14276

r=asuth with changing the getBoundingClientRect().height usage to getBoundingClientRect().top.  (I assume that was what was intended anyways)

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Rebased the patch and fixed the issue from comment 11.  Re-tested on device.

I'm waiting for green travis before merging, but apparently npm is having problems again so it may be delayed even more. :-(

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Finally landed!

  https://github.com/mozilla-b2g/gaia/commit/8bf9d5f14ad81c4a3aba1fdae9611531003681c4

Comment 14

[Dietrich Ayala (:dietrich)]

Blocks a 1.3 CS blocker -> blocking+ to uplift.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Uplifted to v1.3:  b3ddb51c04620a5658c615b1d389478d0a292c64