865743 - SMS main thread list takes too long to load

Status: RESOLVED WONTFIX

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Displaying the list of threads (the first screen when you enter the app) takes way too long when you have a few hundreds of them.

Comment 1

[Lukas Blakk [:lsblakk] use ?needinfo]

Vivien, it looks like this is a placeholder for work on performance but it makes it very hard to block on a bug with no measurable metric for success - can you provide more data on where we are now and what the target it for being able to call this bug 'fixed' on v1.1?

Comment 2

[Alex Keybl [:akeybl]]

Also, is this a regression from v1.0.1?

Comment 3

[David Scravaglieri [:scravag]]

The improvements made on the list component must be applied on all apps that use lists.

Comment 4

[Julien Wajsberg [:julienw]]

Alex, this is not a regression, this has always been the case.

Comment 5

[Kai-Chih Hu [:khu]]

Steve, is this something you could help? Thanks!

Comment 6

[Steve Chung [:steveck]]

It's possible that we can change message threads rendering flow that we display the message one by one from thread cursor instead of display complete thread list at once. But we are preparing to land mms functionality in message app now and it involved some changes in thread list view , we can improve the performance after mms landed for avoiding conflicts. I will discuss this issue with Borja during work week, thanks.

Comment 7

[Julien Wajsberg [:julienw]]

Steve, the purpose of this bug is to render only what's needed, and not the whole list. See the Bug 865741 (see the "depends" line) for more information.

Some preliminary work were done in Bug 859730 (see the depends line there, we still need one fix in the platform), we should probably dupe one of these bugs, but please adjust the dependencies if you dupe.

Comment 8

[Julien Wajsberg [:julienw]]

We probably need to wait after the MMS work lands.

Note that the work in Bug 859730 will probably need to be done again, especially after the MMS changes, but that gives an idea of how this could work.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Now that we've passed FC for v1.1 we are not taking on new perf targets, unless there is an existing target for this particular feature in v1.1 that can be aimed for (as well as partner champion for milestoning and hitting that target in a specific timeframe) this will not be a blocker.

Comment 10

[Corey Frang [:gnarf]]

I would love to work on this and have bug 854417 to adress this problem.

Comment 11

[David Scravaglieri [:scravag]]

Would be great to have performance target for this kind of bug. Not time to take performance patch but definitely tracking.

Comment 13

[Steve Chung [:steveck]]

Just some notes: Although thread display already changed a lot(bug 854417, bug 929919), there still have some room for rendering long threads while start up. Here is some brief profiling for reference-workload-medium/heavy/x-heavy:

workload                            thread db cursor return ~ cursor finished and start to render
medium(89 threads, 500 messages)  : around 700 ~ 800 ms
heavy(180  treads, 1000 mesages)  : around 1500 ~ 1700 ms
x-heavy(578  treads, 2000 mesages): around 4000 ~ 10000 ms

If we can start rendering after sufficient thread (20 or less), the cursor return time to render thread could be reduced to less than 300 ms.

Comment 14

[Gabriele Svelto [:gsvelto]]

(In reply to Steve Chung [:steveck] from comment #13)
> If we can start rendering after sufficient thread (20 or less), the cursor
> return time to render thread could be reduced to less than 300 ms.

You mean start rendering after we've received at least 20 threads? We're currently adding every thread on its own which keeps the app responsive but is pretty slow; batching them up should make the list appear faster indeed at the expense of reduced responsiveness. BTW I think we might want to wait for bug 881469 to land before doing something like this.

Comment 15

[Steve Chung [:steveck]]

(In reply to Gabriele Svelto [:gsvelto] from comment #14)
 
> You mean start rendering after we've received at least 20 threads? We're
> currently adding every thread on its own which keeps the app responsive but
> is pretty slow; batching them up should make the list appear faster indeed
> at the expense of reduced responsiveness. BTW I think we might want to wait
> for bug 881469 to land before doing something like this.

I agree that batching up the threads rendering will also improve the performance and we already applied it in threadUI(message) view. It's also one of the key points in my though. The plan would be:


1) Set the threads number threshold for first-time display in rendering.
2) Create a hidden container to append the thread-list.
3) When thread db cursor starts and traversed threads meets the threshold, start thread appending and clone whole contained thread list from hidden container to thread list view.
4) If use scroll to bottom but the cursor does not complete yet, clone the existing threads in hidden container and append to thread list view. (Seems threaUI view already take similar solution)
5) If cursor traversal complete, clone remaining threads from hidden container to view. (or there is no need to append the lists until user scroll to bottom / in edit mode)

In this plan, we don't have to wait for cursor traversal complete and the view could display the latest few threads at first no matter how big the threads db is. The rendering performance could also improve because we will append thread block by block instead of one by one. Waiting for bug 881469 would be safer because the changes is tremendous, I just want to make sure the plan is accessible and the performance could get some benefits from this patch. Any feedback is welcome for sure! :)

Comment 16

[Gabriele Svelto [:gsvelto]]

+1 and especially this:

(In reply to Steve Chung [:steveck] from comment #15)
> 5) If cursor traversal complete, clone remaining threads from hidden
> container to view. (or there is no need to append the lists until user
> scroll to bottom / in edit mode)

We need better abstractions in the code to achieve it though. A move to a more MVC-like model were operations manipulate the underlying thread/message data instead of the UI would help greatly there. To give an example currently edit mode works on UI elements and not on the underlying data which makes it scale poorly and restricts us from doing things asynchronously as we need things to settle up in the UI first.

Comment 17

[Julien Wajsberg [:julienw]]

see also bug 859730.

Alwo, what we do in the thread view is wrong because this leads to bug 903763.

One of the things to do is to have a model in JS instead of DOM, for example using the existing Threads object. That way we can load all threads in memory without displaying them all (and only display some of them, and display more as we scroll);

Something else that could be very useful and effective: we can put in an asyncStorage key all data that we need to display at launch (a little like we do for Drafts).

Comment 18

[Gabriele Svelto [:gsvelto]]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> see also bug 859730.
> 
> Alwo, what we do in the thread view is wrong because this leads to bug
> 903763.

Thanks for pointing them out, I keep forgetting about bug 903763 in particular and that's important.

> One of the things to do is to have a model in JS instead of DOM, for example
> using the existing Threads object. That way we can load all threads in
> memory without displaying them all (and only display some of them, and
> display more as we scroll);

Yes, yes and yes :)

> Something else that could be very useful and effective: we can put in an
> asyncStorage key all data that we need to display at launch (a little like
> we do for Drafts).

Yeah, that or even better having all the messages in an IndexedDB container in the app; we'd then be able to create all the indices we need to quickly show the latest messages / threads, scroll through them, etc... Heck we could also provide multiple types of user-selectable sorts (oldest first, youngest first, by participant name, etc...). But I digress :)

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to Gabriele Svelto [:gsvelto] from comment #18)
> 
> Yeah, that or even better having all the messages in an IndexedDB container
> in the app; we'd then be able to create all the indices we need to quickly
> show the latest messages / threads, scroll through them, etc... Heck we
> could also provide multiple types of user-selectable sorts (oldest first,
> youngest first, by participant name, etc...). But I digress :)

I think there are different things for different purposes:

1. one db to quickly get the first panel
2. one db to quickly access the messages/threads we want
3. one db to have everything

All 3 are independant. The first one is what I described before, the second one is what you described, the 3rd one is what we have now and could be moved to DataStore one day.

We can do 1 and 2 without bug 881469 ;)

Comment 20

[Gabriele Svelto [:gsvelto]]

Quick update, after having fixed bug 947234 the latency before which we start to display the list has dropped significantly (10+ seconds to less than 0.5s with the x-heavy reference workload). The overall time we take to draw the whole list however is still significant.

A relatively simple fix we could do here is to insert the threads' nodes in the DOM in batches instead of one by one. This should cut down the number of reflows significantly and thus drop the overall draw time by quite a bit. One important thing to keep in mind while doing this is to carefully chose the batch size so that the application remains responsive (scrolling, tapping a thread, writing a new message). Large batch sizes will certainly drop the rendering time significantly but will also make the application unresponsive.

Comment 22

[Doug Sherk (:drs) (inactive)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17920

Taking since I have a patch. This is pretty preliminary work and is far from being the only thing we can do to improve perf here. It's basically an implementation of comment 20.

I did some relatively unscientific testing with the medium workload by using a stopwatch and an Inari. I was unable to get the heavy or x-heavy workloads working for some reason, but I suspect that the results will be even more favorable for them. Here are my findings for the point that the app actually appears (i.e. the placeholder icon goes away, and you can see the app chrome) to the point all threads are loaded and rendered:

With this patch: 11s, 71 reflows
Without this patch: 15s, 215 reflows

I will continue to investigate more ways to eke out some perf here.

Comment 23

[Gabriele Svelto [:gsvelto]]

(In reply to Doug Sherk (:drs) from comment #22)
> I did some relatively unscientific testing with the medium workload by using
> a stopwatch and an Inari. I was unable to get the heavy or x-heavy workloads
> working for some reason, but I suspect that the results will be even more
> favorable for them.

Thanks for doing this Doug, this looks pretty good and I'll try and do some measurements with the profiler tomorrow if I get the time. Note that Yan Or has been doing something similar in bug 972731; he also had taken into account scrolling the list smoothly while loading the threads - which could be a problem while batching up the node insertions - and I'm not sure if your patch also addresses the problem or not.

Comment 24

[Doug Sherk (:drs) (inactive)]

(In reply to Gabriele Svelto [:gsvelto] from comment #23)
> Thanks for doing this Doug, this looks pretty good and I'll try and do some
> measurements with the profiler tomorrow if I get the time. Note that Yan Or
> has been doing something similar in bug 972731; he also had taken into
> account scrolling the list smoothly while loading the threads - which could
> be a problem while batching up the node insertions - and I'm not sure if
> your patch also addresses the problem or not.

Interesting! I'll check out his work and apply his ideas here. From my own testing without actually thinking about this issue, it seemed to be fairly smooth while simultaneously scrolling and loading even the biggest batches.

Comment 25

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8400877 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17920

(In reply to Doug Sherk (:drs) from comment #24)
> Interesting! I'll check out his work and apply his ideas here. From my own
> testing without actually thinking about this issue, it seemed to be fairly
> smooth while simultaneously scrolling and loading even the biggest batches.

Alright, based on my reading of that code, it seems like he spent more time on it and his strategy is better, so we should just use his patch instead. I wish I had known about his work before I started on this.

Comment 26

[Gabriele Svelto [:gsvelto]]

(In reply to Doug Sherk (:drs) from comment #25)
> Alright, based on my reading of that code, it seems like he spent more time
> on it and his strategy is better, so we should just use his patch instead. I
> wish I had known about his work before I started on this.

Sorry for wasting your time, when Yan started working on bug 972731 I forgot to point him here or dup this bug.

Comment 27

[Julien Wajsberg [:julienw]]

Sorry for wasting your time, hopefully you learnt some things in the process!

My own take at this is to do comment 7 at the end...

Comment 28

[GitHub Autolander Bot]

Attachment: [gaia] julienw:865743-batch-display-of-threads > mozilla-b2g:master

Comment 29

[Julien Wajsberg [:julienw]]

Attachment: rebased patch

Rebased patch from Doug.

Thankfully the code barely changed.

I'm gonna try it right now and see how it performs. (at last).

Comment 30

[Julien Wajsberg [:julienw]]

Comment on attachment 8596661 [details] [review]
[gaia] julienw:865743-batch-display-of-threads > mozilla-b2g:master

Ok, I see no improvement with this patch. 
I have 169 threads. Is it too few?

NI Doug to know how many threads are too many threads.

Comment 31

[Doug Sherk (:drs) (inactive)]

I don't remember, but I think I needed hundreds when I last tried this. I was also testing on an Inari, so it's possible that differences between devices have made this strategy less effective.

Comment 32

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(

Comment 33

[Julien Wajsberg [:julienw]]

Mass closing of Gaia::SMS bugs. End of an era :(