Closed
Bug 1117450
Opened 10 years ago
Closed 9 years ago
[Dialer][dolphin][FFOS7715 v2.1] [performance] The time spent on displaying callscreen is much longer after the dial button clicked
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(b2g-v2.1S affected)
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
b2g-v2.1S | --- | affected |
People
(Reporter: arvin.zhang, Assigned: selee)
References
Details
(Keywords: perf, Whiteboard: [sprd388576][dp2])
Attachments
(5 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
text/x-github-pull-request
|
Details | |
(deleted),
text/x-github-pull-request
|
gsvelto
:
review-
|
Details |
*** 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
Updated•10 years ago
|
Whiteboard: [sprd388576] → [sprd388576][dp2]
Comment 1•10 years ago
|
||
It seems that part of this bug is being tracked by bug 1117448. Can we close this as dupe of it?
Flags: needinfo?(arvin.zhang)
Reporter | ||
Comment 2•10 years ago
|
||
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.
Flags: needinfo?(arvin.zhang)
Comment 3•10 years ago
|
||
(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.
Flags: needinfo?(whsu)
Updated•10 years ago
|
blocking-b2g: --- → 2.1S?
Comment 4•10 years ago
|
||
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
Flags: needinfo?(whsu)
Comment 5•10 years ago
|
||
It should be a performance regression, please arrange resource to profile for the bottleneck.
Flags: needinfo?(sku)
Flags: needinfo?(ehung)
Flags: needinfo?(dliang)
Flags: needinfo?(chens)
Comment 6•10 years ago
|
||
NI?whsu to do extra performance comparison. (Flame 512 MB: v1.4 vs v2.1)
Flags: needinfo?(whsu)
Comment 7•10 years ago
|
||
Arvin or William, may we have test result on 7715ea-FFOSv1.4? I think it would be useful to have it.
Flags: needinfo?(arvin.zhang)
Comment 8•10 years ago
|
||
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)
Flags: needinfo?(whsu)
Updated•10 years ago
|
Flags: needinfo?(whsu)
Comment 9•10 years ago
|
||
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
Flags: needinfo?(whsu)
Reporter | ||
Comment 10•10 years ago
|
||
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.
Flags: needinfo?(arvin.zhang)
Comment 11•10 years ago
|
||
(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.
Reporter | ||
Comment 12•10 years ago
|
||
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•10 years ago
|
||
Danny, this should be in your queue for profiling. please take it. thanks.
Assignee: nobody → dliang
Comment 14•10 years ago
|
||
(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•10 years ago
|
||
(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•10 years ago
|
||
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.
Flags: needinfo?(arvin.zhang)
Reporter | ||
Comment 17•10 years ago
|
||
Reporter | ||
Comment 18•10 years ago
|
||
Flags: needinfo?(arvin.zhang)
Reporter | ||
Comment 19•10 years ago
|
||
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
Reporter | ||
Comment 20•10 years ago
|
||
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
Updated•10 years ago
|
blocking-b2g: 2.1S? → 2.1S+
Assignee | ||
Comment 21•10 years ago
|
||
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.
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
Flags: needinfo?(arvin.zhang)
Reporter | ||
Comment 23•10 years ago
|
||
(In reply to Vance Chen [:vchen][vchen@mozilla.com] 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.
Reporter | ||
Comment 24•10 years ago
|
||
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
Flags: needinfo?(arvin.zhang) → needinfo?(selee)
Assignee | ||
Comment 25•10 years ago
|
||
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.
Attachment #8577967 -
Attachment is obsolete: true
Flags: needinfo?(selee)
Reporter | ||
Comment 26•10 years ago
|
||
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));
+ });
},
Assignee | ||
Comment 27•10 years ago
|
||
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.
Attachment #8578599 -
Attachment is obsolete: true
Comment 28•10 years ago
|
||
(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
Flags: needinfo?(dliang)
Updated•10 years ago
|
Flags: needinfo?(chens)
Updated•10 years ago
|
Flags: needinfo?(ehung)
Reporter | ||
Comment 29•10 years ago
|
||
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.
Flags: needinfo?(selee)
Assignee | ||
Comment 30•10 years ago
|
||
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!
Flags: needinfo?(selee)
Reporter | ||
Comment 31•10 years ago
|
||
> 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
Reporter | ||
Comment 32•10 years ago
|
||
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
Attachment #8587171 -
Flags: feedback?(selee)
Assignee | ||
Comment 33•10 years ago
|
||
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.
Attachment #8587171 -
Flags: feedback?(selee) → feedback+
Reporter | ||
Comment 34•10 years ago
|
||
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•10 years ago
|
||
Reporter | ||
Comment 36•10 years ago
|
||
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.
Attachment #8581494 -
Attachment is obsolete: true
Attachment #8587171 -
Attachment is obsolete: true
Attachment #8589470 -
Flags: review?(selee)
Assignee | ||
Comment 37•10 years ago
|
||
Comment on attachment 8589470 [details]
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.
Attachment #8589470 -
Flags: review?(selee) → review?(etienne)
Comment 38•10 years ago
|
||
Comment on attachment 8589470 [details]
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.
Attachment #8589470 -
Flags: review?(etienne) → review?(drs)
Comment 39•10 years ago
|
||
Re-assign to Sean due to he is working on this.
Assignee: danny31012 → selee
ni Doug. Hi Doug, could you help to review the patch?
Flags: needinfo?(sku) → needinfo?(drs)
Comment 41•10 years ago
|
||
Comment on attachment 8589470 [details]
PR for Add mozSettings and CalledNumber cache to optimize the updateCallNumber process
Redirecting again.
Flags: needinfo?(drs)
Attachment #8589470 -
Flags: review?(drs) → review?(gsvelto)
Comment 42•10 years ago
|
||
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•10 years ago
|
||
Comment on attachment 8589470 [details]
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.
Attachment #8589470 -
Flags: review?(gsvelto) → review-
Updated•9 years ago
|
status-b2g-v2.1S:
--- → affected
(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
blocking-b2g: 2.1S+ → ---
Flags: needinfo?(vanceyen)
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•