Closed Bug 844274 Opened 9 years ago Closed 9 years ago

Contact API: Move getAll cache to child

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22
blocking-b2g tef+
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: reuben, Assigned: reuben)

References

Details

Attachments

(2 files, 2 obsolete files)

<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
Attached patch Move cache to child (obsolete) — Splinter Review
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)
Attached patch Move cache to child (obsolete) — Splinter Review
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)
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+
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+
Status: NEW → ASSIGNED
Keywords: checkin-needed
(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
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.
Blocks: 844558
https://hg.mozilla.org/mozilla-central/rev/5f7a0300ac1f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 842975
No longer depends on: 842975
Duplicate of this bug: 842975
This is needed to meet some partner requirements regarding contacts performance.
blocking-b2g: --- → leo?
blocks a blocker
blocking-b2g: leo? → tef+
You need to log in before you can comment on or make changes to this bug.