Contact API: Move getAll cache to child

RESOLVED FIXED in mozilla22

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: reuben, Assigned: reuben)

Tracking

(Blocks: 1 bug)

Trunk
mozilla22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 fixed, b2g18-v1.0.1 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
<sicking> reuben: first get the array from the cache. Then start looping through the array and call mainobjectstore.get() on each item in the array
<sicking> reuben: as the results are coming back from IDB. Immediately send the contact info to the child
<sicking> reuben: in the child, when you get the first contact, return it to the cursor (like today)
<sicking> reuben: in the child, when you get the second contact, add it to a temporary array
<sicking> reuben: note that you may get the second contact to the child before .continue() is called since the parent never waits, it starts sending contacts right away
(Assignee)

Comment 1

5 years ago
Created attachment 717385 [details] [diff] [review]
Move cache to child

This allowed me to simplify a bunch of things. The entire GetNext logic can be removed from the DB, and DB/Service don't have to keep track of cursors anymore. It also allows me to optimize the initial GetAll case when there's no cache.

Initial measurements show time to first contact around 1500ms and time to render 1000 contacts around 7s. This is better than find(), and *much* better than the old behavior, which could take 100+ seconds to render the list.
Attachment #717385 - Flags: review?(jonas)
(Assignee)

Comment 2

5 years ago
Created attachment 717388 [details] [diff] [review]
Move cache to child

Sorry, the other version had debugging leftovers. Interdiff: http://pastebin.mozilla.org/2168416
Attachment #717385 - Attachment is obsolete: true
Attachment #717385 - Flags: review?(jonas)
Attachment #717388 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
Depends on: 836519
Comment on attachment 717388 [details] [diff] [review]
Move cache to child

Review of attachment 717388 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/contacts/ContactManager.js
@@ +630,5 @@
> +    let data = this._cursorData[aCursorId];
> +    if (data.cachedContacts.length > 0) {
> +      if (DEBUG) debug("contact in cache");
> +      let contact = data.cachedContacts.shift();
> +      this._fireSuccessOrDone(data.cursor, contact);

Nit: You can get rid of the temporary here and just do

this._fireSuccessOrDone(data.cursor, data.cachedContacts.shift());

::: dom/contacts/fallback/ContactDB.jsm
@@ +542,5 @@
> +                  aSuccessCb(null);
> +                }
> +              };
> +            }
> +          }.bind(this));

I don't think you need the .bind(this) here since you're not using 'this' inside this function.

::: dom/contacts/tests/test_contacts_getall.html
@@ +289,5 @@
>              ok(true, "result is valid");
>              count++;
>              req.continue();
>            } else {
> +            is(count, 20, "last contact - 20 contacts returned");

Why did this change?
Attachment #717388 - Flags: review?(jonas) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 717418 [details] [diff] [review]
Move cache to child

Thanks for the review!

(In reply to Jonas Sicking (:sicking) from comment #3)
> ::: dom/contacts/tests/test_contacts_getall.html
> @@ +289,5 @@
> >              ok(true, "result is valid");
> >              count++;
> >              req.continue();
> >            } else {
> > +            is(count, 20, "last contact - 20 contacts returned");
> 
> Why did this change?

Before this patch, deleting a contact would remove it from the cache, so existing cursors would not receive it. Now, you always get a snapshot of the contacts as they were when getAll is called.

Comments addressed, r=sicking.
Attachment #717388 - Attachment is obsolete: true
Attachment #717418 - Flags: review+
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Assignee)

Comment 5

5 years ago
(In reply to Reuben Morais [:reuben] from comment #4)
> Before this patch, deleting a contact would remove it from the cache, so
> existing cursors would not receive it. Now, you always get a snapshot of the
> contacts as they were when getAll is called.

<reuben> sicking, child receives second contact, adds it to temporary array. app B deletes second contact. what happens?
<sicking> reuben: we'd still return it. Effectively we return a snapshot of the contacts that existed when getAll was called. I think that's ok.
<sicking> reuben: dealing with the contact DB changing under an app needs more logic anyway
<sicking> reuben: if the app wants to deal with that it'd still need to listen to change notifications and act on those
<sicking> reuben: even with the current behavior the app can end up rendering a contact that is deleted if the contact is removed after the cursor has returned it
<reuben> sicking, not really a snapshot. if app B deletes the third contact instead, it's not going to be returned
<sicking> reuben: it will, since you loop through the contacts immediately they'll be part of the same transaction
<sicking> reuben: so if another app tries to write in between, the write transaction will be stalled until the read transaction is finished. IDB will take care of that
Blocks: 746443
(Assignee)

Comment 6

5 years ago
Performance numbers! (compare to bug 836519 comment 50)

reference-workload-     light     medium       heavy    x-heavy
number of contacts        200        500        1000       2000

First contact:
find                    932ms     1635ms      2888ms     5306ms
getAll (no cache)       516ms     1008ms      1585ms     3167ms
getAll (cache)          327ms      374ms       498ms      764ms

Render all contacts:
find                     2.9s       8.4s       19.9s      54.4s
getAll (no cache)        1.8s       3.9s        7.9s      15.5s
getAll (cache)           1.8s       3.9s        7.7s      15.5s

This makes us better on time to first contact, and way faster on time to render all contacts.
(Assignee)

Updated

5 years ago
Blocks: 844558
https://hg.mozilla.org/mozilla-central/rev/5f7a0300ac1f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22

Updated

5 years ago
Depends on: 842975
(Assignee)

Updated

5 years ago
No longer depends on: 842975
Duplicate of this bug: 842975
This is needed to meet some partner requirements regarding contacts performance.
blocking-b2g: --- → leo?
(Assignee)

Updated

5 years ago
Blocks: 846895
(Assignee)

Comment 11

5 years ago
Created attachment 720689 [details] [diff] [review]
Patch, rebased to mozilla-b2g18
blocks a blocker
blocking-b2g: leo? → tef+
(Assignee)

Updated

5 years ago
status-b2g18: --- → fixed
You need to log in before you can comment on or make changes to this bug.