844274 - Contact API: Move getAll cache to child

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

<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

Comment 1

[Reuben Morais [:reuben]]

Attachment: 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.

Comment 2

[Reuben Morais [:reuben]]

Attachment: Move cache to child

Sorry, the other version had debugging leftovers. Interdiff: http://pastebin.mozilla.org/2168416

Comment 3

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

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?

Comment 4

[Reuben Morais [:reuben]]

Attachment: 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.

Comment 5

[Reuben Morais [:reuben]]

(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

Comment 6

[Reuben Morais [:reuben]]

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.

Comment 7

[Christian Sonne [:cers]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f7a0300ac1f

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f7a0300ac1f

Comment 10

[Gregor Wagner [:gwagner]]

This is needed to meet some partner requirements regarding contacts performance.

Comment 11

[Reuben Morais [:reuben]]

Attachment: Patch, rebased to mozilla-b2g18

Comment 12

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

blocks a blocker

Comment 13

[Reuben Morais [:reuben]]

Comment on attachment 720689 [details] [diff] [review]
Patch, rebased to mozilla-b2g18

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d9278721eea1

Comment 14

[Reuben Morais [:reuben]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/573010de7858