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

RESOLVED FIXED in B2G C3 (12dec-1jan)

Status

P3
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: borjasalguero, Assigned: etienne)

Tracking

({perf})

unspecified
B2G C3 (12dec-1jan)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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

6 years ago
Assignee: nobody → fbsc
blocking-basecamp: --- → ?

Updated

6 years ago
blocking-basecamp: ? → +
(Reporter)

Updated

6 years ago
Depends on: 809661
(Reporter)

Updated

6 years ago
Summary: [SMS] Starting time optimization based on intermediate cache → [SMS] Starting time optimization based on getThreads() method from Gecko.
Borja, does this bug has landed?
(Reporter)

Comment 2

6 years ago
[:vingtetun] Nop yet! I was working last week in FTU. Soon there will be a patch ready for reviewing! ;)

Updated

6 years ago
Component: Gaia → Gaia::SMS
(Reporter)

Updated

6 years ago
Depends on: 814514
Priority: -- → P2
Target Milestone: --- → B2G C2 (20nov-10dec)
(Reporter)

Updated

6 years ago
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: - → ?
(Reporter)

Comment 6

6 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.
Should be ok if bug 813978 is solved
blocking-basecamp: ? → -
Priority: P4 → --
(Reporter)

Comment 8

6 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

6 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: - → ?
Part of SMS perf.
blocking-basecamp: ? → +
Priority: -- → P3

Updated

6 years ago
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.
(Reporter)

Comment 14

6 years ago
Yep, I will release it asap!
(Assignee)

Comment 15

6 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

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

6 years ago
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?
(Reporter)

Comment 20

6 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

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

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

6 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

6 years ago
Assignee: fbsc → nobody
(Assignee)

Comment 26

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

6 years ago
Created attachment 696721 [details] [diff] [review]
Patch proposal

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

6 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)
(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

6 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

6 years ago
Please rebase your code and make the small change requested. R+ for me. Bonne année! Feliz año!
(Reporter)

Comment 33

6 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 on attachment 696721 [details] [diff] [review]
Patch proposal

Borja's review is enough .Thanks!
Attachment #696721 - Flags: review?(21)
(Assignee)

Comment 35

6 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

6 years ago
https://github.com/mozilla-b2g/gaia/commit/7fae1998fab015aa9970b4e78cd7d39c9cc3a0e9
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.