827815 - [SMS][Optimization] Infinite scroll in SMS keeping the cursor.

Status: RESOLVED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

If the list of messages is large given a phone number, we should paginate at Gaia level in order to get best performance and don't get blocking renderings.

Comment 1

[Borja Salguero [:borjasalguero]]

It's related with https://bugzilla.mozilla.org/show_bug.cgi?id=825604, so probably it should have the same status.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is the work that I originally thought was going to be done in bug 813978, which is a blocker.

Without this, even a relatively small number of SMS's in the DB renders the SMS app unusable.  I had to decommission my dogfooding phone because of this.  I would absolutely block the release on fixing this.

Comment 3

[Dietrich Ayala (:dietrich)]

Blocking per comment #2, as well as comments from other partners about performance of SMS views with reasonable numbers of messages (I'm trying to get the numbers they tested with).

Adding qawanted - Can we find out the display times for 100 messages, 500 messages, 1000 messages, etc.

Borja, do you know what needs to be done to fix this? Do you have an estimate for when it can be fixed?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The datapoint that's keeping my dogfood phone switched off is

 165 messages: ~10s to load conversation view

gwagner/fabrice/philikon have a script to preload an SMS DB.

Comment 5

[Gregor Wagner [:gwagner]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> 
> gwagner/fabrice/philikon have a script to preload an SMS DB.

The script can be found in bug 806598.

Comment 6

[Borja Salguero [:borjasalguero]]

> Borja, do you know what needs to be done to fix this? Do you have an
> estimate for when it can be fixed?

I had a patch almost ready, but after talking with Etienne I think that the performance could be even more improved. Tomorrow I will take this task again and I will come back with more details.

Comment 7

[Borja Salguero [:borjasalguero]]

We have two issues here. One is getting a better performance, making a surgical patch for improve this weird behaviour (and this is tracked in https://bugzilla.mozilla.org/show_bug.cgi?id=833855 ), and other one is using the cursor for getting a lazy-loading of messages given a thread. I would like to consider this one tracking-b2g18, and update the status of 833855 to tef+  (there is a patch ready made by Etienne).

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't care to play the flag-jockeying game, but we can't ship a phone with unusable SMS functionality.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I guess that's going to be bug 833855 now?  Alright.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Borja, is performance good enough at this point? All other bugs related to rendering messages before we've loaded them all have been marked fixed.

If performance is still not good enough we need to renominate this bug.

Please keep in mind that we don't need to add complex pagination UI here. We simply need to render 10 or so messages and then slowly insert the remaining ones.

Comment 12

[Borja Salguero [:borjasalguero]]

Hi Jonas,
I was talking with UX about how to improve SMS performance, and different strategies for accomplishing this goal, and they told me that the 'infinite-scrolling' could be the best one, because you load at the beginning a 'chunk' of messages (for example 10), and when you scroll SMS App will render other set of messages and so forth. With the patch of Etienne and with other improvements SMS App is getting a better user experience, so with this latest fix the interaction the loop would be done. I have to polish some details with UX, so I will come back with the details.

Comment 13

[Borja Salguero [:borjasalguero]]

Attachment: PR

Comment 14

[Borja Salguero [:borjasalguero]]

Im working on making the test work again. However we could start reviewing the code and testing the performance meanwhile.

Comment 15

[Julien Wajsberg [:julienw]]

Comment on attachment 712411 [details]
PR

r=me

This makes Sms work with a big volume of data.

If we want to be better than that we'll need to avoid adding the DOM we don't need.

Comment 16

[Julien Wajsberg [:julienw]]

QA should test SMS extensively when this lands because there are a lot of changes in how Sms are rendered.

Comment 17

[Borja Salguero [:borjasalguero]]

Thanks Julien for the review & support. SMS App performance it's gonna let the user get a better user experience for sure! Merging!

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 712411 [details]
PR

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -

User impact if declined: Sms app is unusable when the threads have too many SMS

Testing completed: yes

Risk to taking this patch (and alternatives if risky): medium as lots of things changed. I've tested the most current use cases and this seems to work correctly but we may find other bugs with dogfooders.

String or UUID changes made by this patch: none

Comment 19

[Julien Wajsberg [:julienw]]

master: https://github.com/mozilla-b2g/gaia/commit/8ef87e72f5fa24e5988031a0d9c5e544d1b9404c
(merge commit is https://github.com/mozilla-b2g/gaia/commit/d194b975a843eb6136eef0d8eda7449908285f52)

Comment 20

[Daniel Coloma:dcoloma]

This is a really a great performance improvement that we should seriously consider to land in v1-train. 

Our QA team is testing this to check there are no regressions in master based on risk assessment on comment 18.

Comment 21

[Borja Salguero [:borjasalguero]]

Comment on attachment 712411 [details]
PR

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 22

[rafael.marquez]

The bug has been checked by Telefónica QA team. We executed a small regression on the master branch.

Everything works fine in the master branch, but we will need to retest it when landed in v1-train to ensure the quality.

Comment 23

[Daniel Coloma:dcoloma]

Based on how bad the performance is with more than 30 SMSs in the same thread (more than 2 seconds to start interacting with screen), I am nominating this as a blocker for telefónica.

Comment 24

[Alex Keybl [:akeybl]]

We're approving for landing, but not blocking on this bug. The reasoning is that if this medium-risk patch causes regressions, we'll want to back it out immediately (and may not take another forward fix).

Comment 25

[Julien Wajsberg [:julienw]]

We already know there is a regression but it is already fixed by Bug 841684.

Comment 26

[Julien Wajsberg [:julienw]]

Please don't checkin until Bug 841684 is approved. And then please land both in the same time.

Comment 27

[Alex Keybl [:akeybl]]

Comment on attachment 712411 [details]
PR

Approving for v1-train and v1.0.1, alongside bug 841684.

Comment 28

[John Ford [:jhford] CET/CEST Berlin Time]

v1-train: bbf2b241e478c3f7caa98c61e459772d579d9a68
v1.0.1: f1fb0d896ea0ec7e523cf1c075f23d62d66a8873

Comment 29

[Jason Smith [:jsmith]]

qawanted doesn't appear necessarily anymore - removing keyword, as this was needed for  analysis before development.