972731 - [Messages] Reduce Thread rendering reflows

Status: RESOLVED WONTFIX

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

Description

[Yan Or]

Currently the SMS message container generates one reflow per message,
as each message is rendered separately.

Reduce reflows by grouping multiple messages into a single render.
Initial group size will be smaller to ensure responsive user experience.

Comment 1

[Yan Or]

Attachment: Pull request

Comment 2

[Rick Waldron [:rwaldron]]

Comment on attachment 8376789 [details] [review]
Pull request

Taking the review to help @julienw


G

Comment 3

[Rick Waldron [:rwaldron]]

Whoops, I didn't want to submit that yet. 


Gabriele, do you have any tools for measuring the improvements here?

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Rick Waldron [:rwaldron] from comment #3)
> Gabriele, do you have any tools for measuring the improvements here?

Yes, I can run a profile and compare startup time before and after the patch. I want to ensure that we're not regressing the time it takes to display the first thread. Not clearing the needinfo flag just yet so I won't forget :)

Comment 5

[Rick Waldron [:rwaldron]]

Awesome, that's exactly what I was hoping for :)

Comment 6

[Yan Or]

Comment on attachment 8376789 [details] [review]
Pull request

Minor code fixes after Rick's feedback.  Please advise on when this can be landed after the profiling.

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8376789 [details] [review]
Pull request

feedbacking Gabriele for the profile

Comment 8

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8376789 [details] [review]
Pull request

I've profiled opening the SMS app with the heavy workload, this is a regular profile:

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

This is with the patch applied:

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

The upside here is that with batching enabled the total time required to draw the thread list drops from 4.6s to 3.8s. Frankly I expected more but it's still a nice improvement. The problem however is that with the current batch size (60) it takes over one second to gather a batch and draw it. This makes the app unresponsive to the user which is quite unpleasant; additionally the time it takes to display the first message increases from 1.4s to 1.75s. It might seem a small change but most of our partners are measuring the time it takes for the first message to appear and have fairly strict requirements for it.

To improve upon this I'd say: start with a very small batch size in order not to regress the time-to-show-the-first-message (you could also start with a single message). Then once you went over the first few messages increase the batch size but not by too much; I fear that values above 10 will be too large for smooth operation.

Comment 9

[Rick Waldron [:rwaldron]]

Comment on attachment 8376789 [details] [review]
Pull request

Please r? both me and gsvelto after making changes per his review, thanks!!

Comment 10

[Yan Or]

Change initial batch size to 2, up to first 10 threads.  After that, batch size is increased to 10.

Comment 11

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8376789 [details] [review]
Pull request

Your patch does not apply cleanly to master, could you rebase it please?

Comment 12

[Gabriele Svelto [:gsvelto]]

I've re-profiled your patch against master and here are the results; this is a master run with the heavy reference workload, ~15.5s to display all message threads:

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

This is the same workload using your patch, total time dropped to ~13.1s:

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

This is better than before because the first message is display around the same time as before (the difference looks ~25ms which is not enough to be relevant IMHO) and the following batches also take less time making it possible to scroll the app albeit in a very choppy way.

The downside is that each batch takes ~0.65s to be displayed. This is enough to prevent the app from appearing to have frozen but it's also too much for a smooth response; scrolling is *very* choppy while the list is populated. Also the total time didn't drop as much as I hoped for.

All in all I don't think this improvement is worth the added complexity and higher response times. I would be happier if we'd bite the bullet and implemented an infinite scrolling paradigm instead where we add only the visible threads plus just enough for scrolling in both directions and then dynamically add/remove DOM nodes as the user scrolls up or down. While it would require more effort, it would also fix all of our scalability problems providing quick and fluid response irrespective of the number of threads. 

Rick, what's your take on this?

Comment 13

[Julien Wajsberg [:julienw]]

Gabriele, are you sure you have apzc enabled ? Because scrolling should not be choppy nowadays, even if the app does lots of things...

That said, 2 seconds of improvement is not really worth it, I agree.

Comment 14

[Yan Or]

Comment on attachment 8376789 [details] [review]
Pull request

I looked into the profile data.  Turned out for each thread, a lot of time was spent on getting the contact info.  I think that is dominating the overall time and not reflows.

As an experiment, I've deferred contact rendering until a thread is about to be visible via scrolling.
Time to display all threads seems significantly faster while scrolling is reasonably smooth.

Julien, can you review the code?  I borrowed most of it from the Contact app.

Gabriele, can you rerun profiling on the patch?  Is there a way I can run it myself?

Comment 15

[Julien Wajsberg [:julienw]]

That does not surprise me much :)

Comment 16

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8376789 [details] [review]
Pull request

I finally had some time to profile this again and this is looking *much better*. The time needed to populate the whole list using the heavy workload drops from 15.9s to 13.1s and scrolling the list while it's being populated is smooth and responsive.

Comment 17

[Yan Or]

Gabriele, when you say time to populate whole list, is it the time from launching the app until the list stops updating?  While it's updating, do you scroll the list?  Since scrolling triggers incremental contact info fetch, the overall time will depend on how much scrolling is done.

On my test device and unscientific stopwatch timing, just populating the list without any interaction goes from ~14s to ~7s.

Comment 18

[Julien Wajsberg [:julienw]]

I posted a patch in Bug 837666 today to make this kind of measurement is easier. You might want to apply it on top of master and on top of your patch to have objective measurements.

I won't have time to review this today yet, but tomorrow for sure.

Comment 19

[Gabriele Svelto [:gsvelto]]

(In reply to Yan Or from comment #17)
> Gabriele, when you say time to populate whole list, is it the time from
> launching the app until the list stops updating?  While it's updating, do
> you scroll the list?  Since scrolling triggers incremental contact info
> fetch, the overall time will depend on how much scrolling is done.
> 
> On my test device and unscientific stopwatch timing, just populating the
> list without any interaction goes from ~14s to ~7s.

I did it without scrolling, I measured the time on the profile before the application completely settled down. Since I had loaded the heavy workload for the contacts too I imagine that this included the cost of loading the contacts too. I can re-test w/o the contacts if you wish so. I still find this a very nice improvement BTW.

Comment 20

[Julien Wajsberg [:julienw]]

Was the contacts DB already migrated to the latest DB version? Remember the workload is made for FxOS v1.1.

Comment 21

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> Was the contacts DB already migrated to the latest DB version? Remember the
> workload is made for FxOS v1.1.

Of course, my usual procedure for this test is to load the contacts app first, wait for it to settle down so I'm sure the contacts DB is up to date. Kill it and open the SMS app, wait for it to settle again so that the message DB is also migrated. Kill it and open it again this time with profiling on.

Comment 22

[Julien Wajsberg [:julienw]]

So, I spent lots of time running my patch from Bug 837666 but I couldn't make it report reliable times. Nor that there is much a difference between this patch and what we have on master :(

Actually I don't see much improvement on a real device... Moreover, APZC makes the visibility monitor not work very reliably, and the contacts are fetched very late. I seem to see more checkerboarding with your patch than without.

I don't know how they make the visibility monitor work fine in other apps?

Could you rebase the patch to the current master too, and squash it to one commit? Will make it easier to compare.

Comment 23

[Julien Wajsberg [:julienw]]

Note that I tried with the "heavy" workload for SMS and "medium" for contacts (so that all numbers don't have associated contact).

Comment 24

[Yan Or]

Attachment: New pull request

Comment 25

[Yan Or]

Julien,

I rebased but couldn't get the old patch to squash correctly so I've created a new patch and attached it here.

I'm using a peak phone and heavy load and observed the followings:

Summary:
 Gallery:         100
 Music:           100
 Videos:           20
 Contacts:       1000
 Sms Messages:   1000
 Dialer History:  200
 Calendar:   2400


master:
    start, wait for list to fill - 10.5s
    start, scroll slowly to 6/23, wait for list to fill - 19.5s
    start, scroll quickly to 6/23, wait for list to fill - 20.5s
    start, scroll quickly until list is full - 28s
    
with patch:
    start, wait for list to fill - 7s
    start, scroll slowly to 6/23, wait for list to fill - 16.5s
    start, scroll quickly to 6/23, wait for list to fill - 15.5s
    start, scroll quickly until list is full - 23s

My phone has APZC enabled, I believe.

Comment 26

[Julien Wajsberg [:julienw]]

I probably won't be able to look at it today because of other 1.3+ work but thanks for your rebased patch, I'll compare this again next week!

Comment 27

[Mason Chang [Inactive] [:mchang]]

This looks good, https://people.mozilla.org/~bgirard/cleopatra/#report=d889b720bff512557e89e73ce2651845ea35df53

We save ~100 ms with this patch.

Comment 29

[Julien Wajsberg [:julienw]]

I now see a difference when running the perf test.

Note that in my first test, I used a smaller workload for the "contacts" data, so probably I got a lot less reflows.

I've already spent a lot of time on this today, I'll continue next week.

Comment 30

[Yan Or]

I think smaller contacts workload also means faster lookup from the Contacts API.

Comment 31

[Julien Wajsberg [:julienw]]

Comment on attachment 8383271 [details] [review]
New pull request

Sorry for the delay, I left some comments on github.

I'm trying now on a device to have a new feeling of the improvement.

Comment 32

[Julien Wajsberg [:julienw]]

Now I see a regression in behavior compared to the current master :(

Here is what I see (I compared 2 identical buris with the same Gecko from yesterday, same options):
1) with the patch, the list is actually rendered faster.
2) with the patch, there is a lot of checkerboarding when we scroll (presumably because we trigger reflows when we change a thread)
3) without the patch, the first thread is added quicker.

I'd argue that 2) is more important than 1). And that 3) is more important than 1) too. 1) is not that important because the user will likely want to see the latest messages and not the oldest ones.

Gabriele, I'd like to have a second advice here, can you have a look? I made a rebased commit here: https://github.com/julienw/gaia/commit/f816bab0504ea58c4c8f73343058ff19cd8f2fb4

Maybe idea about how to avoid the checkerboarding would be a plus :)

Comment 33

[Gabriele Svelto [:gsvelto]]

Sorry for the delay here but I couldn't set aside enough time for some exhaustive testing yet so I'm not clearing the needinfo. I did a quick test today with the patch applied and it does seem to introduce some checkerboarding while loading the thread list (I've used the heavy reference workload including contacts) however I need to scroll fairly fast to hit it. I'll try to make some more accurate measures using the profiler also regarding the first thread shown. Anyway IMHO unless checkerboarding becomes a significant problem being able to render the list faster is a significant plus for two reasons:

- the app will become more responsive sooner after launching

- we'll be using less power every time we launch the app; on a device such as the Tarako where the app will get killed pretty much every other time you use it I feel that's important

Comment 34

[Julien Wajsberg [:julienw]]

ok, I don't have the same feeling than you at all :( I guess I'll need to check again...

Comment 35

[Julien Wajsberg [:julienw]]

Comment on attachment 8383271 [details] [review]
New pull request

I think we should part this bug in 2:
* one patch for the chunk rendering
* one patch for the setContact with visibility monitor.

This way we'll more clearly see which one brings improvements where.

Comment 36

[Doug Sherk (:drs) (inactive)]

I'd like to shamelessly plug my patch in bug 865743 which is simpler and may have fewer problems. I haven't tried attachment 8383271 [details] [review], but I know for sure my patch improved things, at least at the time. I'm sorry for drawing attention away from this thread but this is bugging me while dogfooding and I want to fix it.

Comment 37

[Julien Wajsberg [:julienw]]

I'll have a look.

Is your patch trying to fix only one of 2 issues I outlined in comment 35?

Comment 38

[Doug Sherk (:drs) (inactive)]

(In reply to Julien Wajsberg [:julienw] from comment #37)
> I'll have a look.
> 
> Is your patch trying to fix only one of 2 issues I outlined in comment 35?

I'm not sure what the second point means, but it definitely only tackles the first point. I didn't try to do any checking for scroll position to avoid checkerboarding, but I'm not sure how useful that actually is in practice.

Comment 39

[Gabriele Svelto [:gsvelto]]

Clearing my needinfo here because it's been on for a long while and I know I won't have time to look into this for the time being. This is still in my backlog so if I manage to clear some time I'll have a look.

Comment 40

[Julien Wajsberg [:julienw]]

I'm gonna close this. I think we'll be handle this better using a local DB (containing contacts information already) and a service worker.