1117450 - [Dialer][dolphin][FFOS7715 v2.1] [performance] The time spent on displaying callscreen is much longer after the dial button clicked

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[helloarvin]

*** Build Information
Gaia-Rev        17c7ad2e4919a994f0844239b483116090412dee
Gecko-Rev       4f7c43e577a1a43be0af2c02383c1c56c8c4b756
Device-Name     dolphin(ea)

*** Description
The time spent on displaying callscreen is much longer after the dial button clicked.

*** Steps to Reproduce
Pre-conditions: Call-logs(50);Contacts(250);Messages(100);Pictures(300);Songs(150);Videos(150)

1. Click the call button to dial any number and start counting;
2. Stop the timer once the call-screen display completed.

*** Expected Results
The performance of dialing should be the same as 7715ea-android.

*** Actual Results
The time took for showing callscreen after pressed to dial is worse than 7715ea-android about 39%.

The average time
7715ea-FFOSv2.1 ：2.154s
Flame-FFOS  ：1.475s
7715ea-Android: 1.31s

Comment 1

[Beatriz Rodríguez [:brg]]

It seems that part of this bug is being tracked by bug 1117448. Can we close this as dupe of it?

Comment 2

[helloarvin]

Dear Beatriz,

Here we mainly focus on the time consuming of the period between dial button clicked and the callscreen display, and the operation is triggered after the dialer application started. However, the concern in bug1117448 is the long time spent on cold booting of dialer app and it does not meet any overlap with the process of this bug(1117450).

Hence, we should keep on tracking the two different issue by the two bugs.

Contact me if you have any problem.
Thanks a lot.

Comment 3

[Steven Yang [:styang]]

(In reply to helloarvin from comment #2)
> Dear Beatriz,
> 
> Here we mainly focus on the time consuming of the period between dial button
> clicked and the callscreen display, and the operation is triggered after the
> dialer application started. However, the concern in bug1117448 is the long
> time spent on cold booting of dialer app and it does not meet any overlap
> with the process of this bug(1117450).
> 
> Hence, we should keep on tracking the two different issue by the two bugs.
> 
> Contact me if you have any problem.
> Thanks a lot.

ok, I got it. It still does not make sense we got the terrible result on 2.1. ni? William for double check this issue on pvt build.

Comment 4

[William Hsu [:whsu]]

Dolphin v2.1 (512MB) test result.
Visually complete time: 2057.5 ms

@ PVT Build information:
 - Gaia-Rev        2d0df3907319edf55a643b7d4a103534579ebef0
 - Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/2a3804a0911a
 - Build-ID        20150128161200
 - Version         34.0
 - Device-Name     scx15_sp7715ea
 - FW-Release      4.4.2
 - FW-Incremental  eng.cltbld.20150128.193949
 - FW-Date         Wed Jan 28 19:40:01 EST 2015

Comment 5

[Steven Yang [:styang]]

It should be a performance regression, please arrange resource to profile for the bottleneck.

Comment 6

[William Hsu [:whsu]]

NI?whsu to do extra performance comparison. (Flame 512 MB: v1.4 vs v2.1)

Comment 7

[Sherman Chen [:chens] (inactive)]

Arvin or William, may we have test result on 7715ea-FFOSv1.4? I think it would be useful to have it.

Comment 8

[William Hsu [:whsu]]

Summarizing all data.

1. From partner,
  - 7715ea-FFOSv2.1: 2.154s
  - Flame-FFOS:      1.475s
  - 7715ea-Android:  1.31s

2. From Mozilla,
  - 7715ea-FFOSv2.1s (512 MB)： 2.0575s
  - Flame v1.4 (512 MB):        1.353s
  - Flame v2.1 (512 MB):        1.417s

3. Software configuration
  @ Workload: 900 events (Mozilla build)

  @ Build information
   - Flame v1.4 (JB, single-core with 512 MB)
    ~ Gaia-Rev        04265f42139a7a5c611c45e4869582642927e835
    ~ Gecko-Rev       af6e951d18ca
    ~ Build-ID        20150201160228
    ~ Version         30.0

   - Flame v2.1 (KK, single-core with 512 MB)
    ~ Gaia-Rev        63f291df9b9ad8498fb8fc7fb8bf070524406a5c
    ~ Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/70b8982a523d
    ~ Build-ID        20150202161206
    ~ Version         34.0
    ~ Device-Name     flame
    ~ FW-Release      4.4.2
    ~ Bootloader      L1TC100118D0


>>>>>>>>> Comments >>>>>>>>
1. Comparing the dial up time of Flame v2.1 & Flame v1.4, there is no performance regression.
2. Comparing the dial up time of with different devices,
   => Dolphin v2.1s (512 MB) > Flame v2.1 ( single-core with 512MB)

Comment 9

[William Hsu [:whsu]]

Add Dolphin v1.4 (512MB) and Dolphin V1.4 (256MB)

> 1. From partner,
>   - 7715ea-FFOSv2.1: 2.154s
>   - Flame-FFOS:      1.475s
>   - 7715ea-Android:  1.31s
> 
> 2. From Mozilla,
>   - 7715ea-FFOSv2.1s (512 MB)： 2.0575s
    - 7715ea-FFOSv1.4  (512 MB)： 1.207s
    - 7715ga-FFOSv1.4  (256 MB):  1.210s
>   - Flame v1.4 (512 MB):        1.353s
>   - Flame v2.1 (512 MB):        1.417s
> 
> 3. Software configuration
>   @ Workload: 900 events (Mozilla build)
> 
>   @ Build information
>    - Flame v1.4 (JB, single-core with 512 MB)
>     ~ Gaia-Rev        04265f42139a7a5c611c45e4869582642927e835
>     ~ Gecko-Rev       af6e951d18ca
>     ~ Build-ID        20150201160228
>     ~ Version         30.0
> 
>    - Flame v2.1 (KK, single-core with 512 MB)
>     ~ Gaia-Rev        63f291df9b9ad8498fb8fc7fb8bf070524406a5c
>     ~ Gecko-Rev      
> https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/70b8982a523d
>     ~ Build-ID        20150202161206
>     ~ Version         34.0
>     ~ Device-Name     flame
>     ~ FW-Release      4.4.2
>     ~ Bootloader      L1TC100118D0

     - Dolphin v1.4 (512 MB)
       ~ Gaia-Rev        04265f42139a7a5c611c45e4869582642927e835
       ~ Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/93a7fd645e82
       ~ Build-ID        20150204160202
       ~ Version         30.0
       ~ Device-Name     scx15_sp7715ea
       ~ FW-Release      4.4.2

     - Dolphin v1.4 (256MB)
       ~ Gaia-Rev        04265f42139a7a5c611c45e4869582642927e835
       ~ Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/93a7fd645e82
       ~ Build-ID        20150204160202
       ~ Version         30.0
       ~ Device-Name     scx15_sp7715ga
       ~ FW-Release      4.4.2

Comment 10

[helloarvin]

Dear all,

Please pay special attention to the whole process of 'display callscreen':
1) callscreen show;
2) dialing number(or its detailed info: voicemail/contacts-info) turn up.

We can see that it took a little time to turn up the phone number or its detailed info after the shown of callscreen and the period is the integral part of 'display callscreen' in our QA's view.


Dear William,
 Could you please help re-check the time consuming under the above comment?
 Thanks a lot.

Comment 11

[William Hsu [:whsu]]

(In reply to helloarvin from comment #10)
> Dear all,
> 
> Please pay special attention to the whole process of 'display callscreen':
> 1) callscreen show;
> 2) dialing number(or its detailed info: voicemail/contacts-info) turn up.


Our definition. FYI.
The elapsed time beginning when call button is highlighted and ending when the phone number is display on screen.

So, I think the definition is the same as yours if you also count the elapsed time from the highlighted call button.

Comment 12

[helloarvin]

Get it.
Thanks a lot for William's professional description.

And we can see that the difference of the time consuming of 'display callscreen' under the same hardware is very large between the branch v1.4 and v2.1, could someone help check the main cause of the issue?

Thank you all.

Comment 13

[Steven Yang [:styang]]

Danny, this should be in your queue for profiling. please take it. thanks.

Comment 14

[William Hsu [:whsu]]

(In reply to helloarvin from comment #12)
> Get it.
> Thanks a lot for William's professional description.
> 
> And we can see that the difference of the time consuming of 'display
> callscreen' under the same hardware is very large between the branch v1.4
> and v2.1, could someone help check the main cause of the issue?
> 
> Thank you all.

No problem. Our pleasure!

If there is anything we can do to help, please feel free let us know.
Have a nice day! :)

Comment 15

[Beatriz Rodríguez [:brg]]

(In reply to helloarvin from comment #12)
> 
> And we can see that the difference of the time consuming of 'display
> callscreen' under the same hardware is very large between the branch v1.4
> and v2.1, could someone help check the main cause of the issue?
> 
Please investigate this issue and improve dialer performance to avoid issues in certification. Thanks.

Comment 16

[siiaceon]

arvin, pls list the solution discussing this morning to sure the low risk and find the best one (performance, modification, risk) with asking mozilla suggestion.

Comment 17

[helloarvin]

Attachment: remove-voicemail.check-trace.patch

Comment 18

[helloarvin]

Attachment: pure-trace.patch

Comment 19

[helloarvin]

Sorry for update late.

As the above discussion, the whole process should be 'The elapsed time beginning when call button is highlighted and ending when the phone number is display on screen.'(c#11)

So, we can add several necessary trace info and then count the time consuming of every part to check:
1) Logs with pure-trace.patch(Repeat 3 times and select the result close to the average level):

03-02 16:45:43.070 E/GeckoConsole( 1545): Content JS LOG at app://communications.gaiamobile.org/dialer/js/dialer.js:418 in call: moz1117450 --- call number 10086
03-02 16:45:43.600 E/GeckoConsole( 1447): Content JS LOG at app://system.gaiamobile.org/js/dialer_agent.js:170 in da_handleEvent: moz1117450 --- open callscreen for new call.
03-02 16:45:43.620 E/GeckoConsole( 1447): Content JS LOG at app://callscreen.gaiamobile.org/js/handled_call.js:121 in hc_updateCallNumber: moz1117450 --- updateCallNumber enter.
03-02 16:45:43.620 E/GeckoConsole( 1447): Content JS LOG at app://callscreen.gaiamobile.org/shared/js/dialer/voicemail.js:50 in vm_check/<: moz1117450 --- req to get ril.iccInfo.mbdn in voicemail.check.
03-02 16:45:44.690 E/GeckoConsole( 1447): Content JS LOG at app://callscreen.gaiamobile.org/shared/js/dialer/voicemail.js:52 in getVoicemailNumber: moz1117450 --- req.onsuccess in voicemail.check.
03-02 16:45:44.690 E/GeckoConsole( 1447): Content JS LOG at app://callscreen.gaiamobile.org/js/handled_call.js:160 in hc_updateCallNumber/<: moz1117450 --- Voicemail.check.then.
03-02 16:45:44.690 E/GeckoConsole( 1447): Content JS LOG at app://callscreen.gaiamobile.org/shared/js/dialer/contacts.js:34 in findByNumber: moz1117450 --- contacts.findByNumber enter.
03-02 16:45:44.760 E/GeckoConsole( 1447): Content JS LOG at app://callscreen.gaiamobile.org/shared/js/dialer/contacts.js:77 in _findByNumber: moz1117450 --- make request in _findByNumber.
03-02 16:45:44.830 E/GeckoConsole( 1447): Content JS LOG at app://callscreen.gaiamobile.org/shared/js/dialer/contacts.js:79 in findCallback: moz1117450 --- request.onsuccess in _findByNumber.
03-02 16:45:45.020 E/GeckoConsole( 1447): Content JS LOG at app://callscreen.gaiamobile.org/js/handled_call.js:189 in lookupContact: moz1117450 --- lookupContact enter.


From the above logs, we can see that the elapsed time is about 2s and the action of the request on 'ril.iccInfo.mbdn'(voicemail.check[1]) occupied nearly takes half the time(44.690 - 43.620 = 1.07s).

2) Logs with remove-voicemail.check-trace.patch(Repeat 3 times and select the result close to the average level):

03-02 16:52:45.700 E/GeckoConsole( 1931): Content JS LOG at app://communications.gaiamobile.org/dialer/js/dialer.js:418 in call: moz1117450 --- call number 10086
03-02 16:52:46.120 E/GeckoConsole( 1831): Content JS LOG at app://system.gaiamobile.org/js/dialer_agent.js:170 in da_handleEvent: moz1117450 --- open callscreen for new call.
03-02 16:52:46.200 E/GeckoConsole( 1831): Content JS LOG at app://callscreen.gaiamobile.org/js/handled_call.js:121 in hc_updateCallNumber: moz1117450 --- updateCallNumber enter.
03-02 16:52:46.210 E/GeckoConsole( 1831): Content JS LOG at app://callscreen.gaiamobile.org/shared/js/dialer/contacts.js:34 in findByNumber: moz1117450 --- contacts.findByNumber enter.
03-02 16:52:46.210 E/GeckoConsole( 1831): Content JS LOG at app://callscreen.gaiamobile.org/shared/js/dialer/contacts.js:77 in _findByNumber: moz1117450 --- make request in _findByNumber.
03-02 16:52:47.030 E/GeckoConsole( 1831): Content JS LOG at app://callscreen.gaiamobile.org/shared/js/dialer/contacts.js:79 in findCallback: moz1117450 --- request.onsuccess in _findByNumber.
03-02 16:52:47.320 E/GeckoConsole( 1831): Content JS LOG at app://callscreen.gaiamobile.org/js/handled_call.js:191 in lookupContact: moz1117450 --- lookupContact enter.

With the remove-voicemail.check-trace.patch applied, we've removed the action of voicemail.check in the function updateCallNumber. But we can see the elapsed time is still about 1.6s and it takes nearly 0.8s to finish the request of contacts.findByNumber[2].

Above all, we can summarize that it will take nearly 0.5s to notify gaia to open callscreen(received the callstatechange event) after clicked to dial on dolphin for both v1.4 and v2.1(s). Besides, the asynchronous request(mozSettings.createLock().get or mozContacts.find) will always take a long time to finish during call connecting and those time-consuming actions indeed cause the poor performance of call number refresh.

Danny, could you please help research the possile reason based on the above analysis results?

Thanks a lot.

[1] https://github.com/mozilla-b2g/gaia/blob/v2.1s/shared/js/dialer/voicemail.js#L48-L50
[2] https://github.com/mozilla-b2g/gaia/blob/v2.1s/shared/js/dialer/contacts.js#L38-L77
[3] https://github.com/mozilla-b2g/gaia/blob/v2.1s/apps/callscreen/js/handled_call.js#L157-L168

Comment 20

[helloarvin]

Dear Danny,

Per comment #19, I reached a preliminary conclusion that the asynchronous request (mozSettings.createLock().get or mozContacts.find) will always hold an greater proportion of the total elapsed time.

Could you please help research the possible reasons so that we may find out the solutions accordingly, or else we should try to re-factory(fairly large workload) the original flow to optimize the scenario?

Thanks,
Arvin

Comment 21

[Sean Lee [:seanlee][:weilonge]]

Attachment: Trail Patch for v2.1s

Hi all,

This patch is with Settings cache and Call log cache.
I think this patch can improve the performance when making a phone call.
Thank you.

Comment 22

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hello Arvin 

Could you help to try the patch of Comment#21 to see if you observe some improvement of the dialer launch time?

Thanks

Comment 23

[helloarvin]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #22)
> Hello Arvin 
> 
> Could you help to try the patch of Comment#21 to see if you observe some
> improvement of the dialer launch time?
> 
> Thanks

Thanks for your kind support.
I'll extract and apply the patch locally asap.
Keep ni state.

Thanks.

Comment 24

[helloarvin]

Hi Sean,

The patch mainly try to optimize the process of the update call number via introducing the local cache.

1) The re-factory on voicemail.check is quite ideal for we can both observe and store the voicemail numbers in real time, so then we can save the time consuming on the fetching of 'ril.iccInfo.mbdn';

2) We might double check the scheme on _calledNumberCache in contact.js.

2.1) It seems that there's a minor error on the _calledNumberCache in contact.js.

+  _calledNumberCacheGet: function (number) {
+    for (var i = 0; i < this._calledNumberCacheSize; i++) {
+      if (this._calledNumberCache[i] &&
+        number === this._calledNumberCache[i].number) { // @.number
+        return this._calledNumberCache[number];
+      }
+    }
+    return null;
+  },

We cannot use this._calledNumberCache[i].number to compare directly because the value(number) passed into _calledNumberCachePush is an huge object mozContact rather than the pure call number. And so that we can never hit in the cached data.

         var req = fb.getData(contact);
         req.onsuccess = function() {
+          self._calledNumberCachePush(req.result, matchingTel, contactsWithSameNumber);
           callback(req.result, matchingTel, contactsWithSameNumber);
         };
         req.onerror = function() {
           window.console.error('Error while getting FB Data');
+          self._calledNumberCachePush(contact, matchingTel, contactsWithSameNumber);
           callback(contact, matchingTel, contactsWithSameNumber);
         };
       }
       else {
+        self._calledNumberCachePush(contact, matchingTel, contactsWithSameNumber);
         callback(contact, matchingTel, contactsWithSameNumber);
       }

2.2) Besides, it looks like that we cannot detect the modification on the cached contact via the scheme:
a) Edit and save an contact like:{number:10086, name:cmcc}
b) Dial 10086
// we'll store the contact info {number:10086, name:cmcc} into cache
c) Disconnect the 10086, edit the above contact to {number:10086, name:chinamoble}
d) Dial 10086
// we'll hit in the cache and return with 'name:cmcc', the cached contact won't modify unless we dial another 10 different numbers

Above all, could you please help check the minor point and the defect on unable to detect in time?

Thanks a lot.
Arvin

Comment 25

[Sean Lee [:seanlee][:weilonge]]

Attachment: Trail Patch for v2.1s

Hi Arvin,

Thanks for your comments.
I've fixed the issue that you mentioned.
Could you help to try the patch again?

> 2.2) Besides, it looks like that we cannot detect the modification on the
> cached contact via the scheme:
In this patch, contacts changing detection is not implemented yet.
I will try to provide a solution to improve it.

Thank you.

Comment 26

[helloarvin]

Hi Sean,

Thanks for your quick feedback and the updated patch works well on keeping contact cache.

With my local test, the average elapsed time of making call(exclude the first dialing of a number) is still nearly 1.6s. And I found that the window.LazyL10n.get[1] introduced in the 2nd version took about 0.4s(with logs printed), the elapsed time of making call can reach to the level 1.35s-1.4s after the window.LazyL10n.get deleted.

Could you please help check the necessary of window.LazyL10n.get in the function findByNumber? And please help solve the legacy of cached contacts changing detection.

Thanks a lot.

[1] Is it necessary to call LazyL10n in the function findByNumber?
   findByNumber: function findByNumber(number, callback) {
-    LazyLoader.load(this._FB_FILES,
-                    this._findByNumber.bind(this, number, callback));
+    var self = this;
+    window.LazyL10n.get(function () {
+      LazyLoader.load(self._FB_FILES,
+                      self._findByNumber.bind(self, number, callback));
+    });
   },

Comment 27

[Sean Lee [:seanlee][:weilonge]]

Attachment: Trail Patch for v2.1s

Hi Arvin,

LazyL10n is a must for translation, so we can not remove it.
The additional text will be incorrect after removing LazyL10n, so this patch is to load LaztL10n at onload event.

Could you help to try the patch again?
Thank you.

Comment 28

[Danny Liang [:dliang]]

(In reply to Sean Lee [:seanlee] from comment #25)
> Created attachment 8578599 [details]
> Trail Patch for v2.1s
> 
> Hi Arvin,
> 
> Thanks for your comments.
> I've fixed the issue that you mentioned.
> Could you help to try the patch again?
> 
> > 2.2) Besides, it looks like that we cannot detect the modification on the
> > cached contact via the scheme:
> In this patch, contacts changing detection is not implemented yet.
> I will try to provide a solution to improve it.
> 
> Thank you.

Mapping the number to the stage of sprd's log as following:
(1) call number
(2) open callscreen for new call
(3) updateCallNumber enter
(4) req to get ril.iccInfo.mbdn in voicemail.check
(5) req.onsuccess in voicemail.check
(6) Voicemail.check.then
(7) contacts.findByNumber enter
(8) make request in _findByNumber
(9) request.onsuccess in _findByNumber
(10) lookupContact enter

Update my measurement with this patch on eng build with profiling flag:　http://goo.gl/0h1sQ3
1. The bottleneck in comment 19 has been solved, please check google doc for detail.
2. W/ patch, the average time from (1) to (10) is 1.438s, w/o patch is 2.002s. The patch works.
3. W/ patch, there might be less improvement in 1st call, but it improved ~750ms except 1st call.

SW information:
Serial: 09550993818604 (State: device)
Build ID               20150318155630
Gaia Revision          a66813e488374b8ba9d8aa187b00f84ad5bd1bb9
Gaia Date              2015-03-10 06:56:29
Gecko Revision         902afa3f0c48093e91eb2f1878ac913d6e8c84e8
Gecko Version          34.0

Comment 29

[helloarvin]

Attachment: Add-update-cache-feature-based-on-sean's-latest-patch

Hi Sean,

Sorry for my late reply.

With the local check on the attachment 8579249 [details], the average time consuming of making call(excluding the first time dialing a number) is about 1.45s based on three different types of phone number: voicemail number, contact number and other numbers. Obviously, the modification of loading LaztL10n at onload event in advance is very effectual.

  Device: Dolphin7715ea
  Gaia Revision: 2d81e10e2e49b089088f9e93df2beeb2fd2e67ac
  Gecko Revision: b915e4e6cd41bfece441b2c1eeeb0a979dcf6537

Now, Let's back to consider the problem of the cached contacts changing detection mentioned above. This is mainly because we'll always show the 'out-dated' contact info on screen with the current patch under the following scenraio:
a) Create a new contact like:{number:10086, name:cmcc}
b) Dial 10086
// we'll store the contact info {number:10086, name:cmcc} into cache
c) Disconnect the 10086, edit the above contact to {number:10086, name:chinamobile} (or delete the contact)
d) Dial 10086
// we'll hit the '10086' in the cache and display 'chinamobile' on screen

Obviously, the cached contact of the given number '10086' will not be modified to the right version unless the other TEN new numbers dialed. We may imagine that how bad the UE it is for user once occurred. Hence, I tried to improve the terrible case based on your patch in c#27, by introducing the function 'update the cache in time' and the main idea is to add 'create an async-request of the mozContact.find on current number and then update the cache data if needed' when hit in cache. It can guarantee the 'out-dated' cached data only been used once even if there're and will not affect the performance due to the async-request.

Could you please check the draft patch and feel free to contact me if there's any problem.

Thanks a lot.

Comment 30

[Sean Lee [:seanlee][:weilonge]]

Hi Arvin,

Thanks for your effort!!
( Sorry for my late reply too. :(

In case 1, if the contact of user dialing number is changed, will user get the correct contact info at first time?
In my opinion, navigator.mozContacts.oncontactchange can help you to monitor any contact change.

Please see the snippet. It provides a nice way to handle contact change.
https://github.com/weilonge/gaia/blob/v2.1s/apps/communications/dialer/js/call_log.js#L1028

The argument "reason" will be "create", "update", and "delete".
"update" and "delete" should be handled at contact change. (We can ignore "create".)

function requestFindByNumber seems to be refactored from the original code.
If the above suggestion is working, I think the refactor will not be necessary.
(I am not sure your intention of requestFindByNumber here, please correct me if it's wrong)

Thank you!

Comment 31

[helloarvin]

> In case 1, if the contact of user dialing number is changed, will user get
> the correct contact info at first time?
> In my opinion, navigator.mozContacts.oncontactchange can help you to monitor
> any contact change.
> 
> Please see the snippet. It provides a nice way to handle contact change.
> https://github.com/weilonge/gaia/blob/v2.1s/apps/communications/dialer/js/
> call_log.js#L1028
> 
> The argument "reason" will be "create", "update", and "delete".
> "update" and "delete" should be handled at contact change. (We can ignore
> "create".)

[arvin] Sounds perfect! I'll work on this to see if it can solve the problem or not.

> function requestFindByNumber seems to be refactored from the original code.
> If the above suggestion is working, I think the refactor will not be
> necessary.
> (I am not sure your intention of requestFindByNumber here, please correct me
> if it's wrong)

[arvin]According to my original idea, the flow will be:
IF (hit in cache)
   make request to get contact info Asynchronously and then update the cache if needed
ELSE //miss in cache
   make request to get contact info synchronously and then run callback, push the contact into the cache

Hence, the function requestFindByNumber is packaged to satisfy the above two cases and with that we can just pass the different callback function without redundant code.


Above all, I'll update if any developments got.

Thanks,
Arvin

Comment 32

[helloarvin]

Attachment: Trail Patch for v2.1s : Observe via oncontactchange to update _calledNumberCache

Hi Sean,

As our previous discussion, the attached trail patch for v2.1s is available. We observe the modification of contact via mozContacts.oncontactchange and traverse the cached number to check if there's necessary to update(or remove from) the local cache or not.

We should handle all the three reasons('create','update','remove') on a contact change event. The 'create' should not be ignored for the following case:
1) dial 10086;
2) create a new contact : {10086, cmcc}
3) dial 10086
Obviously, the cache won't update and still display 10086 on the step 3 if we ignore the 'create' event. 

Hence, we can conclude the different conditions for the two type operations on the local cache:UPDATE and DELETE.

1) UPDATE (a && b && c)
 a) reason is 'create' or 'update'
 b) cached number equals to the contact's
 c) cached id is null or cached id equals to the contact's

2) DELETE (d || e)
 d) reason is 'remove' and cached id equals to the contact's
 e) reason is 'update' and its number's been changed for the same id

Could you please help check the updated patch and let me know if anything needs to be improved.

Thanks a lot.

Arvin

Comment 33

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8587171 [details] [diff] [review]
Trail Patch for v2.1s : Observe via oncontactchange to update _calledNumberCache

Hi Arvin,

Your patch is good to me.
I leave a comment at Github for a minor coding style suggestion.

Thanks a lot for your effort.

Comment 34

[helloarvin]

Hi Sean,

Thanks for your professional advice.

As you said, we had better to describe the case "create" and "update" rather than "!== remove" so  as to improve the readability and extensibility. I'll modify it soon.

Thanks,
Arvin

Comment 35

[GitHub Autolander Bot]

Attachment: [gaia] arvin-zhang:bug1117450_v2.1s > mozilla-b2g:v2.1s

Comment 36

[helloarvin]

Attachment: PR for Add mozSettings and CalledNumber cache to optimize the updateCallNumber process

Hi Sean,

I've pushed the modification to the github and create a PR accordingly.

Could you please help review the jointly developed patch and land it into the branch v2.1s?

Thanks a lot.

Comment 37

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8589470 [details] [review]
PR for Add mozSettings and CalledNumber cache to optimize the updateCallNumber process

Hi Etienne,
Because I am not the callscreen owner, could you help to review it?
Thanks.

Comment 38

[Etienne Segonzac (:etienne)]

Comment on attachment 8589470 [details] [review]
PR for Add mozSettings and CalledNumber cache to optimize the updateCallNumber process

Forwarding to more active dialer developers.
But I wonder if we're note masking platform issues here.

Comment 39

[Danny Liang [:dliang]]

Re-assign to Sean due to he is working on this.

Comment 40

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

ni Doug. Hi Doug, could you help to review the patch?

Comment 41

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8589470 [details] [review]
PR for Add mozSettings and CalledNumber cache to optimize the updateCallNumber process

Redirecting again.

Comment 42

[Gabriele Svelto [:gsvelto]]

A few thoughts about the changes before I do a proper review:

- This is introducing two separate caches for contacts and settings. I cannot strive enough how likely this is to introduce regressions due to the cache getting out of sync.

- As Etienne pointed out it seems to me that we're indeed hiding some platform issues which makes me wonder why those weren't investigated.

- If I understand correctly this is a v2.1s-only patch, but does this problem also happen in master? If it does then it might be worthy looking into.

Now two things stand out immediately: the first one is that when calling Contacts.init() you don't check the version number of the contacts with mozContacts.getRevision(). This means that if the contacts change between two init() calls (and with the callscreen code not running) the cache will be de-synchronized. The second issue is races: asyncStorage.get() is asynchronous and there's nothing preventing _calledNumberCacheGet() from being called before init() has finished initializing the contact cache.

Comment 43

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8589470 [details] [review]
PR for Add mozSettings and CalledNumber cache to optimize the updateCallNumber process

r- because I see two major issues (besides some nits which I've commented on). The code dealing with the contacts is not checking nor tracking the contacts' revision (mozContacts.getRevision()) which means it will fall out of sync if the contacts change when the oncontactchange handler is not registered. The second issue is that this comes without unit-tests and since I fear this code might introduce regressions it's of paramount importance that we have proper coverage. I know that writing unit-tests is annoying but this is a big change which will not be on master so we need to be extra careful not to break this branch because there won't be much other checking.

Comment 44

[Josh Cheng [:josh]]

Hi Vance,
Do we still care this one for 2.1S?

Comment 45

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Josh Cheng [:josh] from comment #44)
> Hi Vance,
> Do we still care this one for 2.1S?

I think partner already enhance the performance by themselves so it is no longer needed in 2.1s, however, for future ELS, I think we still need to enhance the performance of caller screen

flip the flag per my comments