Closed
Bug 860607
Opened 12 years ago
Closed 12 years ago
[SMS] When deleting 100 conversations in SMS app, SMS app takes too long time to delete
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect, P1)
Tracking
(blocking-b2g:leo+, b2g18 fixed, b2g18-v1.0.1 wontfix)
People
(Reporter: leo.bugzilla.gaia, Assigned: chucklee)
References
Details
(Keywords: perf, Whiteboard: c= s=2013.05.31 , [TD-11593][awaiting decision from dcoloma])
Attachments
(1 file)
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•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?
blocking-b2g: leo? → leo+
status-b2g18:
--- → affected
status-b2g18-v1.0.1:
--- → ?
Keywords: qawanted
Updated•12 years ago
|
Assignee: nobody → janjongboom
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.
Updated•12 years ago
|
Assignee: janjongboom → nobody
Updated•12 years ago
|
Target Milestone: --- → Leo QE1 (5may)
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.
Assignee: nobody → chulee
Updated•12 years ago
|
QA Contact: carlos.martinez
Updated•12 years ago
|
blocking-b2g: leo+ → tef?
Updated•12 years ago
|
Whiteboard: [TD-11593] → [TD-11593][awaiting decision from dcoloma]
Comment 6•12 years ago
|
||
Partners suggested this wasn't critical for v1.0.1 - back to leo+.
blocking-b2g: tef? → leo+
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.
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)
Comment 9•12 years ago
|
||
chuck, would you post your Gaia patch here ? :) (unless it is somewhere else already ?)
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.
Updated•12 years ago
|
Priority: -- → P1
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?
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.
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.
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.
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).
Updated•12 years ago
|
Whiteboard: [TD-11593][awaiting decision from dcoloma] → c= [TD-11593][awaiting decision from dcoloma]
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.
Attachment #747892 -
Flags: feedback?(schung)
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.
Updated•12 years ago
|
Assignee: chulee → schung
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.
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.
Depends on: 871905
Updated•12 years ago
|
Assignee: schung → chulee
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.
Attachment #747892 -
Flags: review?(felash)
Attachment #747892 -
Flags: feedback?(schung)
Attachment #747892 -
Flags: feedback+
Comment 23•12 years ago
|
||
Comment on attachment 747892 [details]
Pull Request for updating Message App.
r=me
works fine, thanks !
Attachment #747892 -
Flags: review?(felash) → review+
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.
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!
Comment 26•12 years ago
|
||
master: cdc37c5b0580b0f97735555b3de5e1bdd9fda08f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
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
Comment 28•12 years ago
|
||
Will do it tomorrow, I have a clear idea on why this happens.
Flags: needinfo?(felash)
Comment 29•12 years ago
|
||
v1-train: 12f80303011646bece5034e901429e3328d6c218
Flags: needinfo?(felash)
Updated•11 years ago
|
Flags: in-moztrap-
Updated•11 years ago
|
Keywords: perf
Whiteboard: c= [TD-11593][awaiting decision from dcoloma] → c= , [TD-11593][awaiting decision from dcoloma]
Updated•11 years ago
|
Whiteboard: c= , [TD-11593][awaiting decision from dcoloma] → c= s=2013.06.14 , [TD-11593][awaiting decision from dcoloma]
Updated•11 years ago
|
Whiteboard: c= s=2013.06.14 , [TD-11593][awaiting decision from dcoloma] → c= s=2013.05.31 , [TD-11593][awaiting decision from dcoloma]
You need to log in
before you can comment on or make changes to this bug.
Description
•