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)
Tracking
(blocking-basecamp:+)
People
(Reporter: borjasalguero, Assigned: etienne)
References
Details
(Keywords: perf)
Attachments
(1 file)
(deleted),
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.
Reporter | ||
Updated•12 years ago
|
Assignee: nobody → fbsc
blocking-basecamp: --- → ?
Updated•12 years ago
|
blocking-basecamp: ? → +
Reporter | ||
Updated•12 years ago
|
Summary: [SMS] Starting time optimization based on intermediate cache → [SMS] Starting time optimization based on getThreads() method from Gecko.
Comment 1•12 years ago
|
||
Borja, does this bug has landed?
Reporter | ||
Comment 2•12 years ago
|
||
[:vingtetun] Nop yet! I was working last week in FTU. Soon there will be a patch ready for reviewing! ;)
Updated•12 years ago
|
Component: Gaia → Gaia::SMS
Updated•12 years ago
|
Priority: -- → P2
Target Milestone: --- → B2G C2 (20nov-10dec)
Why does a bb- styling bug block a major bb+ startup optimization? Typo?
Comment 4•12 years ago
|
||
No provided rationale for blocking. Re-nom if should hold the whole release back.
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: - → ?
Reporter | ||
Comment 6•12 years ago
|
||
[: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.
Comment 7•12 years ago
|
||
Should be ok if bug 813978 is solved
blocking-basecamp: ? → -
Priority: P4 → --
Reporter | ||
Comment 8•12 years ago
|
||
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.
Reporter | ||
Comment 9•12 years ago
|
||
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: - → ?
Updated•12 years ago
|
Target Milestone: B2G C2 (20nov-10dec) → B2G C3 (12dec-1jan)
Comment 11•12 years ago
|
||
Any news on this bug?
This bug is preventing me from dogfooding.
All dependent bugs are fixed. Presumably this just needs Borja's gaia patch and we can resolve.
Reporter | ||
Comment 14•12 years ago
|
||
Yep, I will release it asap!
Assignee | ||
Comment 15•12 years ago
|
||
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
Reporter | ||
Comment 16•12 years ago
|
||
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
Comment 17•12 years ago
|
||
(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...
Assignee | ||
Comment 18•12 years ago
|
||
I'll dot the review then, starting with bug 806352
Comment 19•12 years ago
|
||
(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?
Reporter | ||
Comment 20•12 years ago
|
||
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)
Assignee | ||
Comment 21•12 years ago
|
||
(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.
Comment 22•12 years ago
|
||
(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...
Assignee | ||
Comment 23•12 years ago
|
||
(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.
Comment 24•12 years ago
|
||
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)
Reporter | ||
Comment 25•12 years ago
|
||
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?
Reporter | ||
Updated•12 years ago
|
Assignee: fbsc → nobody
Assignee | ||
Comment 26•12 years ago
|
||
(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.
Comment 27•12 years ago
|
||
(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?
Assignee | ||
Comment 28•12 years ago
|
||
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)
Assignee | ||
Comment 29•12 years ago
|
||
(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)
Comment 30•12 years ago
|
||
(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?
Reporter | ||
Comment 31•12 years ago
|
||
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+
Reporter | ||
Comment 32•12 years ago
|
||
Please rebase your code and make the small change requested. R+ for me. Bonne année! Feliz año!
Reporter | ||
Comment 33•12 years ago
|
||
(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 34•12 years ago
|
||
Comment on attachment 696721 [details] [diff] [review]
Patch proposal
Borja's review is enough .Thanks!
Attachment #696721 -
Flags: review?(21)
Assignee | ||
Comment 35•12 years ago
|
||
(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 !
Assignee | ||
Comment 36•12 years ago
|
||
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.
Description
•