Closed Bug 847404 Opened 12 years ago Closed 12 years ago

Add object store to RecentsDBManager database to host groups of calls

Categories

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

defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18+ fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 + fixed

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

(Keywords: verifyme, Whiteboard: QARegressExclude)

Attachments

(1 file, 2 obsolete files)

We can easily improve the performance of the call log by storing the groups of calls instead of creating them while rendering the view.
Assignee: nobody → ferjmoreno
Attached patch WIP (obsolete) — Splinter Review
Attached patch WIP (obsolete) — Splinter Review
It still needs some work, but this patch contains the basic idea. It would be great if you could give me some feedback before I keep working on it. Thanks!
Attachment #720800 - Attachment is obsolete: true
Attachment #721323 - Flags: feedback?(etienne)
Attachment #721323 - Flags: feedback?(21)
Blocks: 848027
Summary: Add object store to host groups of calls → Add object store to RecentsDBManager database to host groups of calls
Comment on attachment 721323 [details] [diff] [review] WIP Review of attachment 721323 [details] [diff] [review]: ----------------------------------------------------------------- we need this badly. Do you plan on using the "contact" field in the initial version? (I think we should take the time an do it all at once ;)) ::: apps/communications/dialer/js/recents_db.js @@ +5,4 @@ > _dbName: 'dialerRecents', > + _dbRecentStore: 'dialerRecents', > + _dbGroupStore: 'dialerGroups', > + _dbVersion: 3, Not sure: why do we need to bump the version by 2? We can't create/populate in one go? @@ +83,5 @@ > + // number: <String>, > + // contact: <String>, > + // date: <Date>, > + // type: <String>, > + // retryCount: <Number> } after our discussions, I think naming this |count| our |size| would actually be clearer, it's the size of the group after all. @@ +316,5 @@ > + date: (Date.now() - i * 86400000), > + type: type, > + number: number > + }; > + this.add(recent); I would love to extract this in another file. and then we can just un-comment it when we want to work with fake data. Better yet patch, use the make reference-workload. But it might be outside of the scope here :) @@ +325,5 @@ > } > }, > + > + // TODO: This methods are only kept as a temporary meassure for not breaking > + // the call log until bug XXXXX lands. and what XXXXX will be? :) ::: apps/communications/dialer/test/unit/recents_db_test.js @@ +11,5 @@ > > suiteSetup(function() { > + // Avoid prepopulating the database as we want to manage custom and > + // and controlled data. > + RecentsDBManager.prepopulated = true; another argument to decouple the pre-population from the actual code :)
Attachment #721323 - Flags: feedback?(etienne) → feedback+
Attachment #721323 - Flags: feedback?(etienne)
Attachment #723418 - Flags: review?(etienne)
Thanks for the feedback Etienne! (In reply to Etienne Segonzac (:etienne) from comment #3) > Comment on attachment 721323 [details] [diff] [review] > WIP > > Review of attachment 721323 [details] [diff] [review]: > ----------------------------------------------------------------- > > we need this badly. > > Do you plan on using the "contact" field in the initial version? (I think we > should take the time an do it all at once ;)) I do! The current PR includes the 'updateGroupContactInfo' that allows to modify the contact name or number of an existing group. Also, the 'add' function allows to include the contact name within the recent call object to be stored. > > ::: apps/communications/dialer/js/recents_db.js > @@ +5,4 @@ > > _dbName: 'dialerRecents', > > + _dbRecentStore: 'dialerRecents', > > + _dbGroupStore: 'dialerGroups', > > + _dbVersion: 3, > > Not sure: why do we need to bump the version by 2? > > We can't create/populate in one go? Old code :\ I modified an index of the already existing object store in a previous version of the patch, so I needed to do the db upgrade in two steps. Anyway, the current PR has only one step for the db upgrade. > > @@ +83,5 @@ > > + // number: <String>, > > + // contact: <String>, > > + // date: <Date>, > > + // type: <String>, > > + // retryCount: <Number> } > > after our discussions, I think naming this |count| our |size| would actually > be clearer, it's the size of the group after all. > Actually, I would prefer to keep this parameter as 'retryCount', if you don't mind. I might have not explained myself properly about it. The way I conceived this parameter it does not hold the size of the whole group but the size of the consecutive latest calls of the same type. For example, with this calls sequence the group object would look like (with the not existing 'size' member meaning the total size of the calls belonging to the group): - Receive 'incoming' call: { type = 'incoming', retryCount = 1, size = 1 } - Receive 'incoming' call: { type = 'incoming', retryCount = 2, size = 2 } - Send 'dialing' call: { type = 'dialing', retryCount = 1, size = 3 } - Receive 'incoming' call: { type = 'incoming', retryCount = 1, size = 4 } Note that 'size' and 'retryCount' are different. As I commented in the actual code: "'retryCount' is incremented when we store a call of the same 'type' and resetted to 1 otherwise." > @@ +316,5 @@ > > + date: (Date.now() - i * 86400000), > > + type: type, > > + number: number > > + }; > > + this.add(recent); > > I would love to extract this in another file. and then we can just > un-comment it when we want to work with fake data. > > Better yet patch, use the make reference-workload. > > But it might be outside of the scope here :) > I removed that code and I'll file a bug for adding it to the reference workload. > @@ +325,5 @@ > > } > > }, > > + > > + // TODO: This methods are only kept as a temporary meassure for not breaking > > + // the call log until bug XXXXX lands. > > and what XXXXX will be? :) > :D bug 848027
Comment on attachment 723418 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8576 I'd also would love to hear Borja's feedback since he is the one that is going to work in bug 848027.
Attachment #723418 - Flags: feedback?(fbsc)
Attachment #721323 - Attachment is obsolete: true
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5) > > @@ +316,5 @@ > > > + date: (Date.now() - i * 86400000), > > > + type: type, > > > + number: number > > > + }; > > > + this.add(recent); > > > > I would love to extract this in another file. and then we can just > > un-comment it when we want to work with fake data. > > > > Better yet patch, use the make reference-workload. > > > > But it might be outside of the scope here :) > > > > I removed that code and I'll file a bug for adding it to the reference > workload. > Actually, this has been done in https://github.com/mozilla-b2g/gaia/pull/8546
Comment on attachment 723418 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8576 r=me with the comments on github answered/addressed. Thanks again, this is huge :)
Attachment #723418 - Flags: review?(etienne) → review+
Comment on attachment 723418 [details] Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/8576 Hi Fernando, The patch looks great, but I would like to retrieve the results sorted by date, or even better, to have any way of making the request with a 'sortedBy' and ASC/DESC . Thanks for your support with Call log! I think that this change it's a key one for getting a better performance in Comms App. Gracias!
Attachment #723418 - Flags: feedback?(fbsc) → feedback+
Thanks for the feedback! I've addressed Etienne's review comments, added the 'sortedBy' and 'prev' options to db record getters [1][2] and added a few more tests. [1] https://github.com/mozilla-b2g/gaia/pull/8576/files#L0R411 [2] https://github.com/mozilla-b2g/gaia/pull/8576/files#L0R528
https://github.com/mozilla-b2g/gaia/commit/f72bef5aa2536b5d2abb4a61cfd0af83ff00f64e Once bug 848027 is done and we have a proper QA validation, we should probably ask for tef+ for landing these improvements.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Perf improvements => tef? and tracking?
blocking-b2g: --- → shira?
tracking-b2g18: --- → ?
blocking-b2g: shira? → tef?
(tef- because I think we're generally content with call log perf for v1.0.1, definitively should land on leo IMO)
blocking-b2g: tef? → -
blocking-b2g: - → leo?
Our perf targets for v1.1 aren't different than v1.0.1 ferjm - what's the risk around taking this fix in future versions? We'll track/untrack depending.
blocking-b2g: leo? → -
I'm curious about why we don't have an occurrences field in the current store instead of creating another store for groups ...
Triage drive-by: waiting for response to question in comment 14
Flags: needinfo?(ferjmoreno)
(In reply to Alex Keybl [:akeybl] from comment #14) > Our perf targets for v1.1 aren't different than v1.0.1 > > ferjm - what's the risk around taking this fix in future versions? We'll > track/untrack depending. Right now the call log performance is pretty bad. As you can see in Bug 845274 it can even become unusable with a few hundred of calls. We landed a temporary hack to limit the call log to show only the calls from the last week, which makes the dialer usable (but still slow) again (unless you have hundreds of calls during the last week, which seems possible) but it seems like a very bad UX to only allow one week of calls history. The risk of taking this specific fix is low cause this patch still keeps the old methods used in the current implementation of the call log UI. However, this is the first piece for a complete refactor needed to improve the performance of the call log, which is as risky as a refactor can be. Bug 848027 will be applying the changes done in this bug to the recent calls database and hopefully will provide significant performance improvements. We can wait for Bug 848027 to be fixed before landing this patch. As you can see in the attached patch, we are writing a significant battery of tests and we'll be asking QA verification before landing this (along with Bug 848027) in other branches.
Flags: needinfo?(ferjmoreno)
(In reply to Jose M. Cantera from comment #15) > I'm curious about why we don't have an occurrences field in the current > store instead of creating another store for groups ... Sorry, but I can't see how an ocurrences field might solve the current issue :(. Could you elaborate more on this, please? The way I see it, having an ocurrences field in the current store would not save us from (repeatedly) spending time creating the final groups of calls that are shown in the call log UI. Note that the grouping is currently done by number, day and type of call.
We can track and get this verified and come back for an approval nomination once that's provided.
Keywords: verifyme
blocking-b2g: - → leo?
Makes sense to get this uplifted as a portion (or all) of the fix for bug 857398, blocking.
blocking-b2g: leo? → leo+
Uplifted f72bef5aa2536b5d2abb4a61cfd0af83ff00f64e to: v1-train: 1037c612d61f41492ee6086bd4980254c485b761
Blocks: 847406
Flags: in-moztrap-
Whiteboard: QARegressExclude
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: