Closed Bug 808819 Opened 12 years ago Closed 12 years ago

[SMS] Starting time optimization based on getThreads() method from Gecko.

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect

Tracking

(blocking-basecamp:+)

Status:
RESOLVED FIXED
Milestone:
B2G C3 (12dec-1jan)
Project Flags:
blocking-basecamp +

People

(Reporter: borjasalguero, Assigned: etienne)

References

Details

(Keywords: perf)

Attachments

(1 file)

Patch proposal
8.80 KB, patch
borjasalguero
: review+
Details | Diff | Splinter Review
After talking with [:philikon] and [:ferjm] we are going to create an intermediate cache for optimizing the way of painting and updating threads in thread list.
Assignee: nobody → fbsc
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
Depends on: 809661
Summary: [SMS] Starting time optimization based on intermediate cache → [SMS] Starting time optimization based on getThreads() method from Gecko.
[:vingtetun] Nop yet! I was working last week in FTU. Soon there will be a patch ready for reviewing! ;)
Component: Gaia → Gaia::SMS
Depends on: 814514
Priority: -- → P2
Target Milestone: --- → B2G C2 (20nov-10dec)
Depends on: 815260
Why does a bb- styling bug block a major bb+ startup optimization? Typo?
No provided rationale for blocking. Re-nom if should hold the whole release back.
blocking-basecamp: + → -
Keywords: perf
Priority: P2 → P4
The performance of the conversation view on my testdriver phone is unusably horrendous. It takes 10 seconds or more to load a view with O(100) messages. I just consolidated bug 806594 which is the symptom that this bug intends to fix. I don't particularly care if we reopen that and keep it bb+ and this one bb-.
blocking-basecamp: - → ?
[:cjones] The patch in Gaia is ready, but we cant apply it until https://bugzilla.mozilla.org/show_bug.cgi?id=814514 will be fixed (due to applying the patch will produce a lot of regression bugs, with 'getThreadList' would be impossible to delete a Thread... :( ). We would need support on Gecko side in order to apply all changes asap and get a non-horrible experience in SMS App when having a lot of threads.
Should be ok if bug 813978 is solved
blocking-basecamp: ? → -
Priority: P4 → --
This is not related with https://bugzilla.mozilla.org/show_bug.cgi?id=813978 . This bug is related with applying 'getThreadList' to SMS App in Gaia. It's currently blocked by https://bugzilla.mozilla.org/show_bug.cgi?id=814514.
David, this is not related with bug 813978. It should be bb+ due to it's one of the main improvements to be done in SMS App, and it's related with applying a new method available in Gecko to Gaia.
blocking-basecamp: - → ?
Part of SMS perf.
blocking-basecamp: ? → +
Priority: -- → P3
Target Milestone: B2G C2 (20nov-10dec) → B2G C3 (12dec-1jan)
This bug is preventing me from dogfooding.
All dependent bugs are fixed. Presumably this just needs Borja's gaia patch and we can resolve.
Yep, I will release it asap!
Stealing because I plan on dogfooding tonight, and new year's eve is always a good real life SMS app stress test :)
Assignee: fbsc → etienne
Hahaha, dont steal so fast!!! Because my patch is done and it's waiting only for merging other parts of the code and rebasing!
Assignee: etienne → fbsc
(In reply to Borja Salguero [:borjasalguero] from comment #16) > Hahaha, dont steal so fast!!! Because my patch is done and it's waiting only > for merging other parts of the code and rebasing! Can you attach the code here? This code needs to land by tonight...
I'll dot the review then, starting with bug 806352
(In reply to Vivien Nicolas (:vingtetun) from comment #17) > (In reply to Borja Salguero [:borjasalguero] from comment #16) > > Hahaha, dont steal so fast!!! Because my patch is done and it's waiting only > > for merging other parts of the code and rebasing! > > Can you attach the code here? This code needs to land by tonight... Also what merged are you waiting for for asking for a revview I don't see any dependents bug that has not landed?
There are changes coming here https://github.com/mozilla-b2g/gaia/pull/7244 ... As you could see my code has to be rebased and tested again! Because it's not only the request to Gecko (because this performance has been incremented with latest changes in Gecko) it's also about managing DOM in a better way. Probably we could move deadline to C4 due to these changes are not straightforward and I would like to ensure that everything is working properly. Wdyt? We could ask David as well.
Flags: needinfo?(dscravaglieri)
(In reply to Borja Salguero [:borjasalguero] from comment #20) > There are changes coming here https://github.com/mozilla-b2g/gaia/pull/7244 > ... As you could see my code has to be rebased and tested again! Because > it's not only the request to Gecko (because this performance has been > incremented with latest changes in Gecko) it's also about managing DOM in a > better way. Probably we could move deadline to C4 due to these changes are > not straightforward and I would like to ensure that everything is working > properly. Wdyt? We could ask David as well. Well if we choose to go with just implementing what the bug says, I have a surgical bug just for this ready.
(In reply to Borja Salguero [:borjasalguero] from comment #20) > There are changes coming here https://github.com/mozilla-b2g/gaia/pull/7244 > ... As you could see my code has to be rebased and tested again! Can you give a link to your code so it can be seen first ? > Because > it's not only the request to Gecko (because this performance has been > incremented with latest changes in Gecko) it's also about managing DOM in a > better way. How? This bug is about using getThreads reading at the summary. > Probably we could move deadline to C4 due to these changes are > not straightforward and I would like to ensure that everything is working > properly. Wdyt? We could ask David as well. It can't be C4. This prevent people from dogfooding...
(In reply to Etienne Segonzac (:etienne) from comment #21) > Well if we choose to go with just implementing what the bug says, I have a > surgical bug just for this ready. Of course I meant surgical patch.
This issue was due to C2, then C3. Can't wait C4. If you have a patch put it in review quick please.
Flags: needinfo?(dscravaglieri)
This issue was C2,then C3... because Gecko was failing until 5 days ago, so it was impossible to test due to the mess in 'SMS Status' and so forth. [:etienne] If you want we could apply your patch for this bug, but we have to take into account that fixing only this it's only solving the half of the problem. Please create a new bug for 'Optimizing DOM for getAllThreads' and assign to me and I will apply my patch there, wdyt?
Assignee: fbsc → nobody
(In reply to Borja Salguero [:borjasalguero] from comment #25) > This issue was C2,then C3... because Gecko was failing until 5 days ago, so > it was impossible to test due to the mess in 'SMS Status' and so forth. > [:etienne] If you want we could apply your patch for this bug, but we have > to take into account that fixing only this it's only solving the half of the > problem. Please create a new bug for 'Optimizing DOM for getAllThreads' and > assign to me and I will apply my patch there, wdyt? Sounds like the best solution, will do.
(In reply to Borja Salguero [:borjasalguero] from comment #25) > This issue was C2,then C3... because Gecko was failing until 5 days ago, so > it was impossible to test due to the mess in 'SMS Status' and so forth. > [:etienne] If you want we could apply your patch for this bug, but we have > to take into account that fixing only this it's only solving the half of the > problem. Please create a new bug for 'Optimizing DOM for getAllThreads' and > assign to me and I will apply my patch there, wdyt? Can you still provide a link to your code so we can see how you have fix this and what is your DOM changes?
Attached patch Patch proposal — — Splinter Review
Borja, don't know if you have time to review this, if not Vivien is volunteering.
Assignee: nobody → etienne
Attachment #696721 - Flags: review?(fbsc)
Attachment #696721 - Flags: review?(21)
(In reply to Borja Salguero [:borjasalguero] from comment #25) > Please create a new bug for 'Optimizing DOM for getAllThreads' and > assign to me and I will apply my patch there, wdyt? Here it is: bug 825604 (we already have a meta bug for SMS perf BTW, it's bug 806592)
(In reply to Etienne Segonzac (:etienne) from comment #29) > (In reply to Borja Salguero [:borjasalguero] from comment #25) > > Please create a new bug for 'Optimizing DOM for getAllThreads' and > > assign to me and I will apply my patch there, wdyt? > > Here it is: bug 825604 > > (we already have a meta bug for SMS perf BTW, it's bug 806592) Borja can you also explain what is the part of optimization that you win with getThreadList and what is the part that you gain with your DOM changes?
Comment on attachment 696721 [details] [diff] [review] Patch proposal Review of attachment 696721 [details] [diff] [review]: ----------------------------------------------------------------- ::: apps/sms/js/sms.js @@ +183,5 @@ > + getThreads: function mm_getThreads(callback, extraArg) { > + var request = navigator.mozSms.getThreadList(); > + request.onsuccess = function onsuccess(evt) { > + var threads = evt.target.result; > + callback(threads, extraArg); if(callback) for keeping the same structure of 'getMessages'
Attachment #696721 - Flags: review?(fbsc) → review+
Please rebase your code and make the small change requested. R+ for me. Bonne année! Feliz año!
(In reply to Vivien Nicolas (:vingtetun) from comment #30) > (In reply to Etienne Segonzac (:etienne) from comment #29) > > (In reply to Borja Salguero [:borjasalguero] from comment #25) > > > Please create a new bug for 'Optimizing DOM for getAllThreads' and > > > assign to me and I will apply my patch there, wdyt? > > > > Here it is: bug 825604 > > > > (we already have a meta bug for SMS perf BTW, it's bug 806592) > > Borja can you also explain what is the part of optimization that you win > with getThreadList and what is the part that you gain with your DOM changes? With 'getThreadList' we get directly the list of threads, that is so fast compare with analyze all the messages and get the threads of these messages (imagine list of 4000 messages...). In DOM side the major change is that when a message is sent/received/deleted we re-render the thread view, making a request to Gecko and repainting again. WIth DOM performance this 'thread view' will be only updated, so we are gonna optimize Gecko request and repaintings.
Comment on attachment 696721 [details] [diff] [review] Patch proposal Borja's review is enough .Thanks!
Attachment #696721 - Flags: review?(21)
(In reply to Borja Salguero [:borjasalguero] from comment #32) > Please rebase your code and make the small change requested. R+ for me. > Bonne année! Feliz año! Rebased PR with comment addressed https://github.com/mozilla-b2g/gaia/pull/7259 Bonne année !
https://github.com/mozilla-b2g/gaia/commit/7fae1998fab015aa9970b4e78cd7d39c9cc3a0e9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: