845274 - Dialer app is unusable after a few hundreds calls

Status: RESOLVED FIXED

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

Description

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The current call log implementation makes the dialer application unusable after a few hundred calls. I have missed some calls because of the call log trying to update itself when the visibility of the application change and clicking on the call log tab scares me now.

I should not be scared of clicking somewhere on my phone so let's tef? on it. (Also I should probably not missed call because of the call log).

With 396 calls on my db it takes 5.5 seconds to render the list and a few more seconds before it is usable. 

What I believe should be done is to store a limited computed version of the call log instead of a infinite raw version that translate to html at runtime and then makes hundreds of requests to the phone number lib / mozContacts.

I strongly believe we can not ship a phone that is unusable after a few calls :/

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Daniel do you think this should be fixed for 1.0.1 ?

Comment 2

[Daniel Coloma:dcoloma]

Yes, this sounds really bad. Can we just simply automatically purge the call log? E.g. only store a maximum of X days or X calls. I think this is what other devices do.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Daniel Coloma:dcoloma from comment #2)
> Yes, this sounds really bad. Can we just simply automatically purge the call
> log? E.g. only store a maximum of X days or X calls. I think this is what
> other devices do.

I have hundreds of calls for probably the last 6 months in my android phone and that works smoothly. The calls here are just for the month of January!

The issue is the current algorithm, it takes seconds to have a usable usable list and the worse thing is that it did it on every app opening as well as every time the app become visible.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note that all the data here is stored in IndexedDB. So it should be very possible to get the data in an incremental way and render the log asynchronously, or using a "show more" button. I.e. no platform API changes should be needed here.

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

Yeah, definitely some kind of infinite/endless scroll functionality should be included. There are anyhow some difficulties to implement this taking into consideration that right now the calls are stored as they arrive in the IndexedDB but when shown there are grouped by day, by phone number / contact and by type (incoming, outgoing, missed). There are 2 options here:
1) To pre-calculate this grouping as calls take place and keep this grouping also in DB (of course, deleting calls from the Call Log should also be considered for this grouping). This would not only speed up the rendering (no grouping would be needed) but also simplify the implementation of some kind of infinite/endless scrolling since we could retrieve from the DB fixed amounts of rows to show.
2) To keep things as they are regarding the storing of the calls using IndexedDB, get a a fixed amount of calls from the IndexedDB, group them and show them as we are doing right now. In case there is more space available in the screen, repeat the process until there is no blank space available and from this time on apply the infinite/endless scroll functionality.

Another place for improvement is to reduce the times the Call Log is rendered. Right now, we re-render the whole Call Log (this means collecting all the data from the IndexedDB, grouping and displaying it) every time a new call is made or received, every time the user modifies a contact and probably in some other cases I may be missing right now. This is highly inefficient. In fact, in my opinion, the IndexedDB should only be accessed for retrieval when the app is started (or scrolled if we include this functionality) and from that time on the Call Log UI should be updated as calls come and go or any other related update is made (updating contacts, changing the device date, etc.).

If you do not tell me otherwise, I'll start from the latest, this is to reduce the Call Log rendering to the maximum and then we can consider the best infinite/endless scrolling solution to implement.

Comment 6

[Germán Toro del Valle (:gtorodelvalle)]

Additional notes for me and to keep everyone updated ;-) :
- Recents.render(recents) -> renders the call log, this means grouping the calls by day, phone number / contact and type.
      - Called from:
         1. Recents.refresh()
            - Called from:
                1. Recents.init() -> Executed once during the app starting process.
                2. Recent's mozvisibilitychange listener -> Every time the Comms app gains visibility after loosing it. A more intelligent incremental loading from the IndexedDB could be implemented (since the time the Comms app lost visibility).
                3. dialer.js' handleRecentAddRequest(entry) -> Every time a new call registry is included into the IndexedDB. The UI should be incrementally updated, no whole retrieval and rendering needed.
         2. Recents.deleteSelectedRecents() -> When deleting entries from the Call Log. No whole retrieval and rendering needed. We should cover the case when the deletion goes wrong, which right now is not even covered.

   - Recents.updateContactDetails() -> Updates the contact details of every entry in the Call Log.
      - Called from:
         1. Recents.render(recents) -> See above.
         2. Each time the #recents-view tab is selected (dialer.js) -> To cover the case when some contact may have been updated. Considering the possibility to let the Contacts app notify through messages (since it is running in an iFrame) the main Comms app about this fact (contact update) to only update the contact details of the modified contacts (not all the contacts details of all entries of the Call Log).

Comment 7

[Alex Keybl [:akeybl]]

(In reply to gtorodelvalle from comment #5)
> Yeah, definitely some kind of infinite/endless scroll functionality should
> be included.

This bug should focus completely on the simplest solution, which is Daniel's comment 2. Let's file a followup for anything more complicated.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alex Keybl [:akeybl] from comment #7)
> (In reply to gtorodelvalle from comment #5)
> > Yeah, definitely some kind of infinite/endless scroll functionality should
> > be included.
> 
> This bug should focus completely on the simplest solution, which is Daniel's
> comment 2. Let's file a followup for anything more complicated.

Limiting the number will still make the dialer have a big latency even 50 entries so this is far from solving anything and will still make the user miss calls.  This bug should focus on making it usable, it you want to turn this one into a meta-bug and open smaller bugs on how to make it this way that's fine too.

The current algorithm needs some love and anything working around that will not solve the issue here which is missing calls (and currently I can open the dialer app sometimes, it consumes too much memory when opening and trying to display the call log and the app is killed by the kernel before i get any chance to interact with it).

Also even if we don't want to implement a cache there is not a lot of risks regarding the current impl to change the way of constructing the list which is one of the root cause of the issue here.

Comment 9

[Germán Toro del Valle (:gtorodelvalle)]

Yeah Vivien, we are currently working on it from distinct strategies ;-) We'll keep you posted.

Comment 10

[alberto.pastor]

Hi all,

We are working on make the call log rendering in most performant way and avoiding repainting the whole list on new events.
We'll apply:

- Render refactor
- Using oncontactschange event
- Infinite scrolling

Unfortunately, we´ll need more time for avoiding regressions, so we'll work on fixing this asap next week.

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Another improvement we can make is UI virtualization. Which means only a small number of elements are needed to display a large set of data. It reduces the great amount of memory usage and also the response time.

Comment 12

[Borja Salguero [:borjasalguero]]

Attachment: Pull Request

Comment 13

[Andreas Gal :gal]

This bug is critical. The vendors are reporting unacceptable delays bringing up the dialer during a call. Thats a certification issue.

Comment 14

[Andreas Gal :gal]

Also, can we completely avoid populating the recent call list until the user actually clicks on that tab?

Comment 15

[Borja Salguero [:borjasalguero]]

The patch is working with this idea. Only we are going to render the 'call log' when tapping on the tab, not when making/receiving calls.

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 720679 [details]
Pull Request

Etienne this PR adds back recent_db.js to the main code path of the dialer app. Let's see what you're thinking about it.

Comment 17

[Germán Toro del Valle (:gtorodelvalle)]

Requesting info by Etienne to know his thoughts (please see previous comment by Vivien)... ;-)

BTW, I did not want to make some noise here but I though suggesting lazy loading it to the placing when this library is firstly used ;-)

Comment 18

[Etienne Segonzac (:etienne)]

If I understand correctly, we use to load nothing at launch but then load _and_ render the call log after the first call or when the tab was selected.

This patch compromise by loading a bit at launch but not rendering _anything_ until the tab is selected and then rendering a limited amount of entries.

Sounds like a good compromise for now, especially since we know all the good things coming from bug 847404 :)

Comment 19

[Borja Salguero [:borjasalguero]]

Merged!
https://github.com/mozilla-b2g/gaia/commit/fad15e7f0111e18ce9d0bdfc1d2ff6b79b4a9678
https://github.com/borjasalguero/gaia/commit/c358d393cd37c701fc517946b669615d491bbbe7

Comment 20

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 fad15e7f0111e18ce9d0bdfc1d2ff6b79b4a9678
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train)

Comment 21

[Germán Toro del Valle (:gtorodelvalle)]

v1-train: https://github.com/mozilla-b2g/gaia/commit/149350eaa2d85228166566fd884a07763444e550

Comment 22

[Germán Toro del Valle (:gtorodelvalle)]

v1.0.1: https://github.com/mozilla-b2g/gaia/commit/ef39356bb074a8f8404fd578b2b6558e418fcb8a

Comment 23

[Chris Kreinbring [:CKreinbring]]

Can you please provide steps to verify this fix as we will blackbox test from the UI?

Comment 24

[Germán Toro del Valle (:gtorodelvalle)]

Hi ckreinbring! I would suggest to load some data into the device and try the Call Log. To load this data you can use: "make reference-workload-medium"

If you have any doubt or problem, please do not hesitate to contact me. Thanks!

Comment 25

[Darren]

Cannot verify, this would be easier to check if done by automation.

Comment 26

[Germán Toro del Valle (:gtorodelvalle)]

I am not currently aware of any other means to automatically include 100 Call Log entries :( other than making the calls themselves, of course... :-) Let me invite Fernando to comment on this but I am afraid that the prepopulate() functionality has been deleted from recents_db.js, right?

Comment 27

[Fernando Jiménez Moreno [:ferjm]]

(In reply to gtorodelvalle from comment #26)
> I am not currently aware of any other means to automatically include 100
> Call Log entries :( other than making the calls themselves, of course... :-)
> Let me invite Fernando to comment on this but I am afraid that the
> prepopulate() functionality has been deleted from recents_db.js, right?

We do have a way to pre-populate the call log via "make reference-workload" as Germán mentioned before https://github.com/mozilla-b2g/gaia/tree/master/test_media/reference-workload

Also there are already performance tests for the call log that you can check at https://datazilla.mozilla.org/b2g/?branch=master&range=7&test=Dialer/callLog_rendering_time_%3E_call-log-ready&app_list=browser,calendar,camera,clock,communications/dialer,contacts,email,fm_radio,ftu,gallery,marketplace,messages,music,phone,settings,usage,video&app=communications/dialer&gaia_rev=fcfe81c9181e39e8&gecko_rev=303a259d0eada399

However note that these tests landed after this patch, so you might need to run the tests locally or with your own automatic testing infrastructure.

Comment 28

[John Hammink]

STR: 

Populate the call log up to 390 calls; make another 20 calls, verify the call log production works ok and is performant.

Comment 29

[Sreenidhi]

I did a test on the Inari, V1.0.1 with the following build:

Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/dfa6bdc5e6da
Gaia   3cdab45a783efadf9b97decf46bd15b83fb91ed8
BuildID 20130422070204
Version 18.0

Did a make reference-workload-x-heavy (make reference-workload-medium populates the phone upto 100 calls and that works okay). The Call log has been populated with 500 calls. The dialer is taking up quite sometime to render the list and like the reported one, makes the dialer app unusable. I'm not able to tap on the 'Missed'tab to see the missed calls.

Comment 30

[Sreenidhi]

Is the one on comment 29 a V1.0.1 build? I dont think so.

Comment 31

[John Thomas [:Johnt]]

Unable to verify this issue as this is something Qanalyst is unable to verify efficiently. Adding QAExclude to QA Whiteboard.

Comment 32

[Johan Lorenzo [:jlorenzo]]

This patch was landed 2.5 years ago. We have dogfooders that have more than a hundreds calls. I don't think it's necessary to verify this bug anymore. Striking the keyword