860607 - [SMS] When deleting 100 conversations in SMS app, SMS app takes too long time to delete

Status: RESOLVED FIXED

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

Description

[Leo]

1. Title : When deleting 100 conversations in SMS app, SMS app takes too long time to delete
2. Precondition :
 1) Restore 500 conversations
3. Tester's Action : Run SMS App >> Select edit mode icon >> Select 100 conversations >> Select delete icon >> Delete
4. Detailed Symptom : SMS app takes 68.2 seconds(3 times average), however it should be under 1.5 seconds accordint to leo device spec
5. Expected : SMS app takes under 1.5 seconds
6. Reproducibility: Y
1) Frequency Rate : 100%
7. Gaia revision :

Comment 1

[Alex Keybl [:akeybl]]

leo+ for now assuming this is a regression, and qawanted to find out if v1.0.1 is affected. If yes, please mark as blocking-b2g:tef?

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

The current way of deleting is:

1. Grab all thread IDs and make a SmsFilter out of that
2. Get all messages that comply to this filter
3. Delete these messages one by one
4. Retrieve new thread list, now none of the messages show up

I'd suggest we fix this in Gecko by having a 'mass delete' function or something, but my gecko knowledge is not big enough to fix this nicely.

Comment 3

[Julien Wajsberg [:julienw]]

probably depens on bug 771458.

Comment 4

[Ken Chang[:kenkai]]

Chuck, FFOS can only delete one SMS in one deleting SMS request now.  We have to change the deleting  method to delete multiple SMSs in one deleting SMS requestion.

Comment 5

[Carlos Martínez Toral [:carlosmartinez]]

Is happening in V1.0.1 too.

Comment 6

[Alex Keybl [:akeybl]]

Partners suggested this wasn't critical for v1.0.1 - back to leo+.

Comment 7

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to leo.bugzilla.gaia from comment #0)
> 5. Expected : SMS app takes under 1.5 seconds

It takes about 6 seconds to delete 100 SMS in emulator.
And in my WIP patch for Bug 771458, it takes about 3 seconds.
It is much faster, but in proportion, the speed might be around 30 seconds on leo.

Comment 8

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Test result of deleting 100 SMS on Unagi:
Before patch : 36 seconds(100/1300), 23 seconds(100/1200)
After patch : 6 seconds(100/1000, 100/900)

Comment 9

[Julien Wajsberg [:julienw]]

chuck, would you post your Gaia patch here ? :)
(unless it is somewhere else already ?)

Comment 10

[Julien Wajsberg [:julienw]]

oh, or you maybe tested using... tests. Ok, forget me. ;)

Comment 11

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> chuck, would you post your Gaia patch here ? :)
> (unless it is somewhere else already ?)

The first step of speed up deleting SMS is bug 771458, so my test result is based on the patch there.

Corresponding Gaia patch is attached as attachment 740698 [details] [diff] [review].

Marionette test case is not ready yet.

Comment 12

[Kai-Chih Hu [:khu]]

(In reply to Leo from comment #0)
> 1. Title : When deleting 100 conversations in SMS app, SMS app takes too
> long time to delete
> 2. Precondition :
>  1) Restore 500 conversations
> 3. Tester's Action : Run SMS App >> Select edit mode icon >> Select 100
> conversations >> Select delete icon >> Delete
> 4. Detailed Symptom : SMS app takes 68.2 seconds(3 times average), however
> it should be under 1.5 seconds accordint to leo device spec
> 5. Expected : SMS app takes under 1.5 seconds

Should we MUST hit this target for now?

Comment 13

[Leo]

Hi,

The target time is 1.5 sec.

Thanks,
Leo

Comment 14

[Kai-Chih Hu [:khu]]

Sorry if I asked some stupid question here. Is it already an asychronized call here now? If it is, then it should be able to hit 1.5 sec, IMO. But, of course, we may need to take care of it if it's asynchronized. 

Otherwise, since it's much better than before, is it possible to treat this issue as a performance enhancement? Thank you.

Comment 15

[Julien Wajsberg [:julienw]]

khu: where do you see that it's much better than before ? afaik no work has been done to make it better without Bug 771458...

This is an asynchronous call but we display a screen while it's deleting, until it's deleted, so I don't understand why this matters.

Comment 16

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Julien Wajsberg [:julienw] from comment #15)
> khu: where do you see that it's much better than before ? afaik no work has
> been done to make it better without Bug 771458...
> 
> This is an asynchronous call but we display a screen while it's deleting,
> until it's deleted, so I don't understand why this matters.

Based on latest patch of bug 771458, the measured API execution time(from calling mozMobileMessage.delete() to onsuccess() is fired) is ~1.3 seconds on unagi.

We have to make it land so it can be tested on other devices. If that works well, we don't have to apply other patch for this bug.

Comment 17

[Julien Wajsberg [:julienw]]

I agree, this is the good path, but the gaia patch should be done on this bug instead for the sake of clarity, and reviewed by a gaia sms owner (ie: borja or me).

Comment 18

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Attachment: Pull Request for updating Message App.

After bug 771458, now mozMobileMessage.delete() accepts array for deleting multiple SMS, so update Message App to speed up SMS delete.

It's the simplest modification, but I am not sure if is the best practice.
Steve, please give me your opinion, or take it directly.

Comment 19

[Steve Chung [:steveck]]

I add some comment in patch, but I'll take this one because status in message app is not clear not and I'll send another patch for deletion when message app is reopen. Thanks for the suggestion.

Comment 20

[Julien Wajsberg [:julienw]]

Steve, if you're busy with the MMS work I can take this one if you want, just tell me if you need it.

Comment 21

[Steve Chung [:steveck]]

Julien: This issue didn't uplift yet because 871905. We need to make sure both issue land and uplift to b2g before merging the gaia change.

Comment 22

[Steve Chung [:steveck]]

Comment on attachment 747892 [details]
Pull Request for updating Message App.

Hi Chuck, I set the assignee to you because I think the patch works for me. Both deleteMessage and deleteMessages are used in current code base.

Hi Julien, could you please help review this patch? If you think deleteMessage need more refinement, I can take over this issue. Thanks.

Comment 23

[Julien Wajsberg [:julienw]]

Comment on attachment 747892 [details]
Pull Request for updating Message App.

r=me

works fine, thanks !

Comment 24

[Julien Wajsberg [:julienw]]

as a side note, I got this error:
E/GeckoConsole( 4270): [JavaScript Error: "aMessageRecord is undefined" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 673}]

I don't know exactly when, I couldn't reproduce when I tried then. Maybe you would know.

Comment 25

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

(In reply to Julien Wajsberg [:julienw] from comment #24)
> as a side note, I got this error:
> E/GeckoConsole( 4270): [JavaScript Error: "aMessageRecord is undefined"
> {file:
> "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js"
> line: 673}]
> 
> I don't know exactly when, I couldn't reproduce when I tried then. Maybe you
> would know.

I can't tell from that, I'll keep an eye on it. Thanks!

Comment 26

[Julien Wajsberg [:julienw]]

master: cdc37c5b0580b0f97735555b3de5e1bdd9fda08f

Comment 27

[Gareth Aye [:gaye] (back from PTO)]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x  cdc37c5b0580b0f97735555b3de5e1bdd9fda08f
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 28

[Julien Wajsberg [:julienw]]

Will do it tomorrow, I have a clear idea on why this happens.

Comment 29

[Julien Wajsberg [:julienw]]

v1-train: 12f80303011646bece5034e901429e3328d6c218