<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

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.

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

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?

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.

(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.

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

blocks a blocker

Comment on attachment 720689 [details] [diff] [review] Patch, rebased to mozilla-b2g18 https://hg.mozilla.org/releases/mozilla-b2g18/rev/d9278721eea1