Leo Reporter
	Description • 12 years ago

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 :

	Alex Keybl [:akeybl]
	Comment 1 • 12 years ago

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?

	Jan Jongboom [:janjongboom] (inactive)
	Comment 2 • 12 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 3 • 12 years ago

probably depens on bug 771458.

	Ken Chang[:kenkai]
	Comment 4 • 12 years ago

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.

	Carlos Martínez Toral [:carlosmartinez]
	Comment 5 • 12 years ago

Is happening in V1.0.1 too.

	Alex Keybl [:akeybl]
	Comment 6 • 12 years ago

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

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 7 • 12 years ago

(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.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 8 • 12 years ago

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)

	Julien Wajsberg [:julienw]
	Comment 9 • 12 years ago

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

	Julien Wajsberg [:julienw]
	Comment 10 • 12 years ago

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

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 11 • 12 years ago

(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.

	Kai-Chih Hu [:khu]
	Comment 12 • 12 years ago

(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?

	Leo Reporter
	Comment 13 • 12 years ago

Hi,

The target time is 1.5 sec.

Thanks,
Leo

	Kai-Chih Hu [:khu]
	Comment 14 • 12 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 15 • 12 years ago

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.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 16 • 12 years ago

(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.

	Julien Wajsberg [:julienw]
	Comment 17 • 12 years ago

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).

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 18 • 12 years ago

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.

	Steve Chung [:steveck]
	Comment 19 • 12 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 20 • 12 years ago

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

	Steve Chung [:steveck]
	Comment 21 • 12 years ago

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.

	Steve Chung [:steveck]
	Comment 22 • 12 years ago

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.

	Julien Wajsberg [:julienw]
	Comment 23 • 12 years ago

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

r=me

works fine, thanks !

	Julien Wajsberg [:julienw]
	Comment 24 • 12 years ago

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.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Assignee
	Comment 25 • 12 years ago

(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!

	Julien Wajsberg [:julienw]
	Comment 26 • 12 years ago

master: cdc37c5b0580b0f97735555b3de5e1bdd9fda08f

	Gareth Aye [:gaye] (back from PTO)
	Comment 27 • 12 years ago

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

	Julien Wajsberg [:julienw]
	Comment 28 • 12 years ago

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

	Julien Wajsberg [:julienw]
	Comment 29 • 12 years ago

v1-train: 12f80303011646bece5034e901429e3328d6c218