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)
Firefox OS Graveyard
Gaia::Dialer
Tracking
(blocking-b2g:leo+, b2g18+ fixed)
RESOLVED
FIXED
blocking-b2g | leo+ |
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 | ||
Updated•12 years ago
|
Assignee: nobody → ferjmoreno
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Summary: Add object store to host groups of calls → Add object store to RecentsDBManager database to host groups of calls
Comment 3•12 years ago
|
||
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?(21) → feedback?(etienne)
Assignee | ||
Updated•12 years ago
|
Attachment #721323 -
Flags: feedback?(etienne)
Assignee | ||
Comment 4•12 years ago
|
||
Pointer to Github pull-request
Assignee | ||
Updated•12 years ago
|
Attachment #723418 -
Flags: review?(etienne)
Assignee | ||
Comment 5•12 years ago
|
||
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
Assignee | ||
Comment 6•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Attachment #721323 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
(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 8•12 years ago
|
||
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 9•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
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
Assignee | ||
Comment 11•12 years ago
|
||
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
Assignee | ||
Comment 12•12 years ago
|
||
Perf improvements => tef? and tracking?
blocking-b2g: --- → shira?
tracking-b2g18:
--- → ?
Assignee | ||
Updated•12 years ago
|
blocking-b2g: shira? → tef?
Comment 13•12 years ago
|
||
(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? → -
Assignee | ||
Updated•12 years ago
|
blocking-b2g: - → leo?
Comment 14•12 years ago
|
||
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? → -
Comment 15•12 years ago
|
||
I'm curious about why we don't have an occurrences field in the current store instead of creating another store for groups ...
Comment 16•12 years ago
|
||
Triage drive-by: waiting for response to question in comment 14
Flags: needinfo?(ferjmoreno)
Assignee | ||
Comment 17•12 years ago
|
||
(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)
Assignee | ||
Comment 18•12 years ago
|
||
(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.
Comment 19•12 years ago
|
||
We can track and get this verified and come back for an approval nomination once that's provided.
Keywords: verifyme
Assignee | ||
Comment 20•12 years ago
|
||
leo? based on https://bugzilla.mozilla.org/show_bug.cgi?id=857398#c7
blocking-b2g: - → leo?
Comment 21•12 years ago
|
||
Makes sense to get this uplifted as a portion (or all) of the fix for bug 857398, blocking.
blocking-b2g: leo? → leo+
Comment 22•12 years ago
|
||
Uplifted f72bef5aa2536b5d2abb4a61cfd0af83ff00f64e to:
v1-train: 1037c612d61f41492ee6086bd4980254c485b761
status-b2g18:
--- → fixed
Updated•12 years ago
|
Flags: in-moztrap-
Updated•12 years ago
|
Whiteboard: QARegressExclude
You need to log in
before you can comment on or make changes to this bug.
Description
•