887198 - [sms] lazy load the thread view

Status: RESOLVED WONTFIX

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

Description

[Julien Wajsberg [:julienw]]

We should lazy load/lazy remove the actual thread content instead of focussing on attachments only, using getMesssages with a timestamp-based filter. This could help with the general memory consumption too.

Right now we're hiding specific DOM elements until the user scrolls up. This helps with the initial display (because the reflow is faster), but by _not_ adding the DOM at all we would make it really fast.

Taking it

Comment 1

[:gerard-majax]

I did some profiling and hacking on this topic. My SMS database is quite important, > 50MB with 25000 messages, mostly in one big thread. Loading this thread exposes bad behavior:
 - full load takes minutes, even if from a user point of view the only visible impact is the cpu usage
 - memory usage is important: from the threads list where we have a memory usage of ~18MB, once the full thread is loaded I get figures around 120MB

Switching apps in this context is hard: SMS app is becoming more and more unresponsive, taking several seconds (sometimes over 15 secs) to just be ready to handle inputs. This becomes worse as soon as the user send messages in this thread.

This use case may look at the edge, but it's just the result of my dogfooding. So I fear other people can easily reach this.

Comment 2

[:gerard-majax]

Attachment: WIP/Experimental patch

Julien, I tried this change to limit the amount of messages we deal with. One in use on my dogfooding Nexus S, this makes me able to fully use Messages app. As described in the previous comment, this app was becoming totally unusable due to the amount of data.

It's quite a hack, that leverages the SmsFilter object to limit retrieval to the last months. The behavior is to be enabled from Settings, Messaging. One drawback of this approach is that threads with messages older than one month will appear empty. But in the context of a hack/experiment and for my use, it's okay.

The results of applying this patch is that the Messages app is usable gain, with a memory consumption limited to 22MB after fully loading the same giant thread.

Comment 3

[Julien Wajsberg [:julienw]]

Comment on attachment 8428373 [details] [review]
WIP/Experimental patch

This is obviously a hacky hack ;) but thanks for sharing anyway.

Comment 4

[:gerard-majax]

Looks like there is a regression somewhere: my filtering do not work anymore and I'm getting too much messages again. Looking at Gecko, I can see the startDate/endDate are null-valued. I also noticed that bug 878533 may impact this hack.

Comment 5

[:gerard-majax]

Attachment: 0001-Bug-887198-Limit-SMS-MMS-retrieval-to-last-month.patch

Updated hack that broke because of recent changes regarding SMS filtering.

Comment 6

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #4)
> Looks like there is a regression somewhere: my filtering do not work anymore
> and I'm getting too much messages again. Looking at Gecko, I can see the
> startDate/endDate are null-valued. I also noticed that bug 878533 may impact
> this hack.

As documented in comment 5, bug 878533 did change MessageManager.getMessages() hence my hack needed some modifications: startDate and endDate were not copied and sent to MobileMessageDB.

Comment 7

[Julien Wajsberg [:julienw]]

So basically we have bug 903763 happening when we have enough messages in one thread. The issue is that even if we don't display the messages, we still add them to the DOM, which triggers a style recalculation.

There are several ways to fix it:

1. do not add the messages to the DOM (that's bug 903763). Then we need to add them while scrolling up.
2. one way is just to load and add less messages in the view. This was the goal for this bug.

I still think that 1. is doable and not that difficult: we can keep all messages in memory instead of adding them, and when we want to display them (currently we change all their classes) we add them to the DOM instead.

About 2., I got an idea while talking with Alexandre, which would be easier than do a smart scrolling algorithm.

Here is what we can do:
* have a setting that say "don't display messages older than 1 month"
* when displaying a thread, obey this setting
* at the top of the thread, have 2 buttons: one that would load 1 more month, one that would load all messages for this thread
* (possibly) if the last month is empty, load previous months until we get some messages to display

This would save memory as well for sure.

What do you think Jenny?

Comment 8

[Jenny Lee]

Hi Julien,

I prefer 2. without adding any setting UI. Instead of loading messages from a certain time frame, we load a certain *amount* of messages first and display them, when user scrolls up to the top, show a "Load earlier messages" button, once user taps on it, load the same amount of messages and so on so forth (like what whatsapp does). Or, we can do when user scrolls to the top, don't show any button and simply starts loading the same amount of message (like what Line does).  

I don't think user needs to make the decision of "whether or not to show message older than x month" prior to the need for seeing earlier message happens. If user wants to see earlier message, he has to scroll up anyway, we can let user decide then. Hope this makes sense :)

Comment 9

[:gerard-majax]

Jenny, I don't think the current Gecko-side API allows to do the select based on the amount of messages. At least that's why I went throught the road of using a one-month window, because I could not see any easy way to limit in term of "I want the last X messages". Maybe we could just have a sliding window defining startDate/endDate.

Comment 10

[Steve Chung [:steveck]]

(In reply to Alexandre LISSY :gerard-majax from comment #9)
> Jenny, I don't think the current Gecko-side API allows to do the select
> based on the amount of messages. At least that's why I went throught the
> road of using a one-month window, because I could not see any easy way to
> limit in term of "I want the last X messages". Maybe we could just have a
> sliding window defining startDate/endDate.

They don't, but we could do it ourself actually, just make cursor continues the times we want ;)

Comment 11

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #10)
> (In reply to Alexandre LISSY :gerard-majax from comment #9)
> > Jenny, I don't think the current Gecko-side API allows to do the select
> > based on the amount of messages. At least that's why I went throught the
> > road of using a one-month window, because I could not see any easy way to
> > limit in term of "I want the last X messages". Maybe we could just have a
> > sliding window defining startDate/endDate.
> 
> They don't, but we could do it ourself actually, just make cursor continues
> the times we want ;)

Yep; then if we feel we need to have gecko really restrict according to the count, we can ask that to gecko dev, but the bottleneck is not here.


(In reply to Jenny Lee from comment #8)
> Hi Julien,
> 
> I prefer 2. without adding any setting UI. Instead of loading messages from
> a certain time frame, we load a certain *amount* of messages first and
> display them, when user scrolls up to the top, show a "Load earlier
> messages" button, once user taps on it, load the same amount of messages and
> so on so forth (like what whatsapp does). Or, we can do when user scrolls to
> the top, don't show any button and simply starts loading the same amount of
> message (like what Line does).  

I don't usually like the "autoload" algorithms, that's why I proposed a button.
What do you think of the idea of having 2 buttons for "loading everything" and "loading the same amount" ? I think that having a button for "loading the same amount" can be cumbersome for a user that wants to find a particular message when he knows the date, that's why I proposed this 2 buttons solution.

Also, I think it could actually be quite quick to display because we could do it in a synchronous way if the MMS are already parsed and we have all the data in an array (which is not the case at load-time). So maybe we can try having only one button that would simply display everything and see how it behaves?

> 
> I don't think user needs to make the decision of "whether or not to show
> message older than x month" prior to the need for seeing earlier message
> happens. If user wants to see earlier message, he has to scroll up anyway,
> we can let user decide then. Hope this makes sense :)

Yep, I agree.

NI Jenny for the question above (and sorry for the late reply!)

Comment 13

[Jenny Lee]

I think the one button load all approach is pretty reasonable, let's give it a try and see how it performs, thanks!

Comment 14

[:gerard-majax]

Attachment: Limit SMS/MMS retrieval to last month

Comment 15

[:gerard-majax]

Just an update on the status. My thread is now over 30000 messages, and even on powerful devices:
 - the last messages of the threads are visible quickly after I opened it
 - it takes more than 30 secs between sending messages and those to start appearing in the thread, because of the IPC workload undergoing and pulling the whole thread in background
 - each time I lock/unlock my device and go back to the Messages app, it takes 5 secs before the application is ready to react to anything

Comment 17

[:gerard-majax]

Adding dogfood keyword. FYI, I'm still being blocked and having to resort to my own local builds because of this issue.

Comment 18

[:gerard-majax]

Attachment: Limit SMS/MMS retrieval to last month

Comment 19

[Doug Sherk (:drs) (inactive)]

Julien, Oleg, can you help get this landed?

Comment 20

[Julien Wajsberg [:julienw]]

Doug, the patch Alexandre share is good for him but it's not good for everybody. It limits all message retrieval but gives no way to retrieve the previous one.

So it's not landable like this.

This is a huge work to make this work properly. No mystery we didn't work on this since it was filed.

We have quite a clear path since comment 15 but you know well how it went since end of October (RTL and ideation and NGA).

So no need to dream and think this will land this week. Sorry.

Comment 21

[:gerard-majax]

Heavy foxfooding users might be hitting this ...

Comment 22

[Julien Wajsberg [:julienw]]

Not before some time though.

Comment 23

[:gerard-majax]

Attachment: Limit SMS/MMS retrieval to last month

Comment 24

[:gerard-majax]

Attachment: Limit SMS/MMS retrieval to last month

Comment 25

[Julien Wajsberg [:julienw]]

Last UX is comment 13.


From the technical point of view, there are several possibilities:

1. Put all unrendered messages in a JS array and render them when the user taps this button.
2. Stop the API loop after <n> messages (likely n == 10 or 20 for example) and rerequest them all when the user taps the button.

2) is likely easier but will likely break edit mode if we don't take care.
1) is maybe already easy with our Threads.js. Also we need to take care to edit mode as well.

Comment 26

[:gerard-majax]

Attachment: Limit SMS/MMS retrieval to last month

Comment 27

[Julien Wajsberg [:julienw]]

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

Comment 28

[Julien Wajsberg [:julienw]]

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