I want to write up a better description of what we want to achieve with thi sand why, but for now I'm attaching this WIP just so my current work is available somewhere. Still quite a few things to do: - Sorting needs to be implemented in ContactDB - The new request on continue() is not intuitive at all. What I really wanted was to have it work just like SmsRequest, but that requires changes to DOMRequest. Needs to be discussed. - Need to figure out what queries are affected by an update and invalidate their caches. Right now the "get 20 contacts" test only returns one because it gets the cache from the early "get 1 contact" test.

sicking, in bug 722626 comment 6 you mention you want to use DOMRequest in place of SmsRequest and IDDBRequest. Was there any progress on the needed spec changes? I really don't want to implement Yet Another DOMRequest-like Object that supports firing multiple events for the same request. Also, I'd like feedback on the getAll API. This is how you get all contacts using it: > function() { > ok(true, "Retrieving 20 contacts with getAll"); > req = mozContacts.getAll({}); > var count = 0; > req.onsuccess = function reqOnSuccess(event) { > ok(event.target.result, "result valid"); > var cursor = event.target.result; > if (cursor.contact) { > ok(true, "result.contact is valid"); > count++; > cursor.continue().onsuccess = reqOnSuccess; // ideally this would be just continue() and the original request's onsuccess would fire again > } else { > is(count, 20, "20 contacts returned"); > next(); > } > } > req.onerror = onFailure; > },

New WIP version, rebased to tip, moved tests to their own file, rudimentary cache invalidation. I'll focus on moving the sorting function into ContactDB next so we can actually see the performance impact on the Contacts app. Filtering options aren't allowed in getAll, needs to be documented. (Unless we want to turn the cache/cursor mechanism into something all find calls use, and get rid of getAll entirely).

Alright, so this version has sorting merged into ContactDB, and it passes all existing tests. It should be good enough to prototype something in Gaia and see the perf difference. Better cache invalidation on contact removal still needs to be done.

Tracking for v1.x, but we won't make a decision for v1.0.0 until this is landed and tested. There's a good chance that the amount of change and risk will be too much for v1.0.0

I couldn't apply this patch. Could you please rebase it? Thanks.

There's no spec here yet. But for now you should use DOMRequestCursor. You can simply rename the existing DeviceStorageCursor to avoid reimplementing anything. You'll probably have to add DOMRequestService::createCursor() and DOMRequestService::fireDone(nsIDOMDOMRequestCursor) though so that you can use the cursor from JS.

Alberto, can you try this patch? It contains both the version attached here and the patch in bug 836513, rebased to mozilla-b2g18 tip.

Hi there, I'm getting a [JavaScript Error: "this is undefined" {file: "resource://gre/modules/ContactDB.jsm" line: 528}] when calling mozContacts.getAll(); from gaia. I tried to replace "this" by "self" and then other errors are coming. Could you please review it? Let me know if I can help with something. Thanks!

(In reply to Alberto Pastor from comment #9) > I'm getting a [JavaScript Error: "this is undefined" {file: > "resource://gre/modules/ContactDB.jsm" line: 528}] when calling > mozContacts.getAll(); from gaia. I tried to replace "this" by "self" and > then other errors are coming. I don't know how you're getting that error, the mochitests exercise the same code path and work fine. Are you calling getAll() exactly like that? You need to pass an empty options object if you don't want any sorting/limitting (mozContacts.getAll({});, see dom/contacts/tests/test_contacts_getall.html)

As we talked offline, I left my tests in https://github.com/albertopq/gaia/tree/dummy-contacts-cursor Waiting for your feedback. Thanks!

With this patch and 1000 contacts in the database the first onsuccess callback fires after ~1700ms.

I forgot to include a change to the schema upgrade code in the previous patch.

All the necessary changes to build this in mozilla-b2g18 in a single patch. Applies cleanly using |git apply| as well.

After applying 710994, I get the following error, just when trying to add contacts using UITest. E/GeckoConsole( 108): [JavaScript Error: "NotFoundError: The operation failed because the requested database object could not be found. For example, an object store did not exist but was being opened." {file: "resource://gre/modules/IndexedDBHelper.jsm" line: 114}] May be I'm doing something wrong. Can somebody verify Contacts works normally with this patch applied?

(In reply to Alberto Pastor from comment #15) > After applying 710994, I get the following error, just when trying to add > contacts using UITest. > > E/GeckoConsole( 108): [JavaScript Error: "NotFoundError: The operation > failed because the requested database object could not be found. For > example, an object store did not exist but was being opened." {file: > "resource://gre/modules/IndexedDBHelper.jsm" line: 114}] > > May be I'm doing something wrong. Can somebody verify Contacts works > normally with this patch applied? Did you apply a previous version of this patch on the same profile? Try resetting your Gaia profile, one of the previous versions had a bug in the upgradeSchema function where it wouldn't create the object store for the cache.

This version is fully functional, and uses the simpler API from the patch in bug 837917. Code using it looks like this: >var cursor = mozContacts.getAll({}); >var count = 0; >dump("Contacts:\n"); >cursor.onsuccess = function() { > if (cursor.result) { > dump("#" + count++ + ": " + req.result.name + "\n"); > cursor.continue(); > } else { > dump("------------\n"); > } >}

Comment on attachment 712024 [details] [diff] [review] Implement nsIDOMContactManager.getAll(), v2 Review of attachment 712024 [details] [diff] [review]: ----------------------------------------------------------------- First comments. ::: dom/base/IndexedDBHelper.jsm @@ +117,5 @@ > > txn.oncomplete = function (event) { > if (DEBUG) debug("Transaction complete. Returning to callback."); > + if (successCb) > + successCb(txn.result); nit: {} @@ +131,5 @@ > + if (event.target.error) > + failureCb(event.target.error.name); > + else > + failureCb("UnknownError"); > + } nit: {} ::: dom/contacts/ContactManager.js @@ +330,5 @@ > ContactManager.prototype = { > __proto__: DOMRequestIpcHelper.prototype, > _oncontactchange: null, > > + cursorData: {}, lets do _cursorData @@ +388,5 @@ > + case "Contacts:GetAll:Next": > + let cursor = this.cursorData[msg.cursorId]; > + let contact = msg.contact ? this._convertContact(msg.contact) : null; > + if (cursor.readyState === "done") { > + Services.DOMRequest.fireDone(cursor, contact); Shouldn't we delete the cursor from cursorData? @@ +578,5 @@ > + if (DEBUG) debug("filter options are ignored by getAll!"); > + aOptions.filterBy = undefined; > + aOptions.filterOp = undefined; > + aOptions.filterValue = undefined; > + } We should change this. Maybe use a BasicFindOptions and ExtendedFindOptions. Still not perfect but we shouldn't use it like this. ::: dom/contacts/fallback/ContactDB.jsm @@ +486,5 @@ > + let req; > + try { > + req = store.get(aObjectId); > + } catch (e) { > + Math.sin(0.2); Maybe we should remove this now :) ::: dom/contacts/tests/test_contacts_getall.html @@ +110,5 @@ > + url: [{type: ["work", "work2"], value: "www.1.com"}, {value:"www2.com"}], > + anniversary: new Date("2000, 12, 01"), > + sex: "male", > + genderIdentity: "test" > +}; You don't use them in there

Time from getAll to first contact using the code in https://github.com/reubenmorais/gaia/tree/dummy-contacts-cursor is: 1000 contacts, no cache: 1700ms 1000 contacts, cache: 250ms 2000 contacts, no cache: 2000ms 2000 contacts, cache: 250ms

\o/ So just to make sure everybody is on the same page here. The very fist time you start the contacts app after importing them from simcard or facebook or ... we have to populate the cache and it will be slow. But every following call will be fast, even if you close the app or restart the phone.

Addressed initial review comments, added the transient cache to deal with the problem of data consistency when someone adds a contact while someone else is iterating through contacts, and added cleanup code to ContactService.jsm so that the caches can be cleaned up when the child is shutdown.

Comment on attachment 712732 [details] [diff] [review] Implement nsIDOMContactManager.getAll(), v3 Review of attachment 712732 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good! But I want to see a new version or my questions answered :) ::: dom/contacts/ContactManager.js @@ +451,5 @@ > + contactID: msg.contactID, > + reason: req.reason > + }); > + this._oncontactchange.handleEvent(event); > + } Hm this shouldn't be here! This is already done in "Contact:Changed" ::: dom/contacts/ContactManager.manifest @@ +10,5 @@ > component {ed0ab260-e4aa-11e1-9b23-0800200c9a66} ContactManager.js > contract @mozilla.org/contactTelField;1 {ed0ab260-e4aa-11e1-9b23-0800200c9a66} > > +component {cb008c06-3bf8-495c-8865-f9ca1673a1e1} ContactManager.js > +contract @mozilla.org/contactSimpleFindOptions;1 {cb008c06-3bf8-495c-8865-f9ca1673a1e1} Didn't you call it contactFindSortOptions? ::: dom/contacts/fallback/ContactDB.jsm @@ +31,5 @@ > __proto__: IndexedDBHelper.prototype, > > + cursorIdToCacheIdMap: {}, > + transientCaches: {}, > + cursorIndex: {}, Why not create a single object that is indexed with the cursorID and holds a duple of cache and current index? @@ +404,5 @@ > + for (let i = 0; i < cache.length; ++i) { > + if (cache[i] === aObjectId) { > + cache.splice(i, 1); > + if (this.cursorIndex[cursorId] === aObjectId) { > + this.cursorIndex[cursorId] = cache[i+1]; what if it was the last object? @@ +416,5 @@ > + for (let cursor in this.transientCaches) { > + let cache = this.transientCaches[cursor]; > + for (let i = 0; i < cache.length; ++i) { > + if (cache[i] === aObjectId) { > + cache.splice(i, 1); The cursorIndex[cursorId] update above happens async and the cache update sync. We update the transientCache but the cursorIndex could still hold the old value. That seems like a race condition to me. @@ +581,5 @@ > + if (aCachedResults[i] === index) { > + this.cursorIndex[aCursorId] = aCachedResults[i+1]; > + return this.getContactById(aCachedResults[i], function (aContact) { > + aSuccessCb(aContact); > + }); Hm I don't like that we have to iterate over the whole array all the time. We can do better here. Maybe store the id and the last index and access directly with the last index? ::: dom/contacts/fallback/ContactService.jsm @@ +130,5 @@ > + function(aContact) { > + if (aContact === null) { > + let cursors = this._liveCursors[mm]; > + cursors.splice(cursors.indexOf(msg.cursorId), 1); > + } aContact === null happens in the last continue call? @@ +209,5 @@ > } > break; > case "child-process-shutdown": > if (DEBUG) debug("Unregister"); > + this._db.releaseCursors(this._liveCursors[mm]); Who deletes this._liveCursors[mm]?

(In reply to Gregor Wagner [:gwagner] from comment #23) > Comment on attachment 712732 [details] [diff] [review] > Implement nsIDOMContactManager.getAll(), v3 > > Review of attachment 712732 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks pretty good! But I want to see a new version or my questions answered > :) > > ::: dom/contacts/ContactManager.js > @@ +451,5 @@ > > + contactID: msg.contactID, > > + reason: req.reason > > + }); > > + this._oncontactchange.handleEvent(event); > > + } > > Hm this shouldn't be here! This is already done in "Contact:Changed" Argh, that was a merge problem. > ::: dom/contacts/ContactManager.manifest > @@ +10,5 @@ > > component {ed0ab260-e4aa-11e1-9b23-0800200c9a66} ContactManager.js > > contract @mozilla.org/contactTelField;1 {ed0ab260-e4aa-11e1-9b23-0800200c9a66} > > > > +component {cb008c06-3bf8-495c-8865-f9ca1673a1e1} ContactManager.js > > +contract @mozilla.org/contactSimpleFindOptions;1 {cb008c06-3bf8-495c-8865-f9ca1673a1e1} > > Didn't you call it contactFindSortOptions? Yep, nice catch. > ::: dom/contacts/fallback/ContactDB.jsm > @@ +31,5 @@ > > __proto__: IndexedDBHelper.prototype, > > > > + cursorIdToCacheIdMap: {}, > > + transientCaches: {}, > > + cursorIndex: {}, > > Why not create a single object that is indexed with the cursorID and holds a > duple of cache and current index? No particular reason other than trying to build the transient cache code in a undo-able way. I'll change it. > @@ +404,5 @@ > > + for (let i = 0; i < cache.length; ++i) { > > + if (cache[i] === aObjectId) { > > + cache.splice(i, 1); > > + if (this.cursorIndex[cursorId] === aObjectId) { > > + this.cursorIndex[cursorId] = cache[i+1]; > > what if it was the last object? |undefined|, handled in getNext() > @@ +416,5 @@ > > + for (let cursor in this.transientCaches) { > > + let cache = this.transientCaches[cursor]; > > + for (let i = 0; i < cache.length; ++i) { > > + if (cache[i] === aObjectId) { > > + cache.splice(i, 1); > > The cursorIndex[cursorId] update above happens async and the cache update > sync. > We update the transientCache but the cursorIndex could still hold the old > value. That seems like a race condition to me. I'll move it into the callback. > @@ +581,5 @@ > > + if (aCachedResults[i] === index) { > > + this.cursorIndex[aCursorId] = aCachedResults[i+1]; > > + return this.getContactById(aCachedResults[i], function (aContact) { > > + aSuccessCb(aContact); > > + }); > > Hm I don't like that we have to iterate over the whole array all the time. > We can do better here. Maybe store the id and the last index and access > directly with the last index? I'd rather just store the index. Accomplishes the same with less code, and was the original solution in previous patches. I switched to ids instead of numerical indexes because it makes the code slightly easier to read. > ::: dom/contacts/fallback/ContactService.jsm > @@ +130,5 @@ > > + function(aContact) { > > + if (aContact === null) { > > + let cursors = this._liveCursors[mm]; > > + cursors.splice(cursors.indexOf(msg.cursorId), 1); > > + } > > aContact === null happens in the last continue call? Yes. I'll add a comment. > @@ +209,5 @@ > > } > > break; > > case "child-process-shutdown": > > if (DEBUG) debug("Unregister"); > > + this._db.releaseCursors(this._liveCursors[mm]); > > Who deletes this._liveCursors[mm]? No one. It's just an array of UUIDs, but I'll do that as well.

We will consider a low risk uplift nomination this week to get this in v1.0.1 but this isn't blocking any blockers and we're being very discriminating about what is a blockers at this point.

Correction, this is marked as blocking 835742, however we're still being conservative about adding more blockers - tracking & uplift nomination will get this viewed when it's ready.

(In reply to Lukas Blakk [:lsblakk] from comment #26) > Correction, this is marked as blocking 835742, however we're still being > conservative about adding more blockers - tracking & uplift nomination will > get this viewed when it's ready. What does that mean? Copy and paste some text here doesn't help much. We need priorities. Should I go and fix other tef+ blockers or should we try to fix this? This is a big change, including other blocking bugs. Can I tell people I need review right now or do I have to wait a few days until they fixed and reviewed other tef+ bugs.

We'll be re-evaluating blockers with partners tomorrow so will revisit bug 835742 - but our aim will be to get as many bugs that are not absolute shipping blockers off the list which will likely include several performance related bugs, hence pushing off several of these to tracking where uplift nominations (low risk, and this week) will be considered but the priority will be blockers left on the list after tomorrow morning (PST) triage.

Performance work is being driven by partner requirements. Bugs like this are key to meeting expectations, so we need to look at more than just risk/reward tradeoffs for these types of bugs.

Comments addressed, and shift() instead of keeping indexes around. Works like a charm :)

Forgot to add nsIDOMDOMCursor.idl before I qref'ed.

Mounir's review comments in bug 837917 required changes to this patch. I'll wait until I get r+ there to r? again here.

Hopefully final version, uses the latest version of the patch in bug 837917.

With changes as discussed on IRC. Got rid of the cursorIdToCacheIdMap, simplifying the removeObjectFromCache logic.

I pushed this to Try: https://tbpl.mozilla.org/?tree=Try&rev=9e222b172ea9

Yet Another Version with changes discussed on IRC.

Sigh.

Comment on attachment 714681 [details] [diff] [review] Implement nsIDOMContactManager.getAll(), v8 Review of attachment 714681 [details] [diff] [review]: ----------------------------------------------------------------- \o/ Thanks! ::: dom/contacts/fallback/ContactDB.jsm @@ +405,5 @@ > + cursor.update(cursor.value); > + break; > + } > + } > + // update the transient caches. I don't think we need this any more. we skip them afterwards. @@ +497,5 @@ > + }, > + > + getContactById: function CDB_getContactById(aContactId, aCallback) { > + this.getObjectById(STORE_NAME, aContactId, aCallback); > + }, Combine them to getEntryById and pass the store. ::: dom/contacts/tests/test_contacts_getall.html @@ +307,5 @@ > + var count = 0; > + var previousId = null; > + req.onsuccess = function() { > + if (req.result) { > + ok(true, "on success"); nit: whitespace

BTW: It's sad that we need 2 transactions to put the result query in the store but I can't think if an easy fix here.

Comments addressed, r=gwagner.

I backed this out: https://hg.mozilla.org/integration/mozilla-inbound/rev/eee6e9f4738c The first push of this: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7a145f17e37c has M2 WinXP opt orange. It's just one so far, but I also have a set of try results of my own, that build upon that changeset (with nothing touching contacts API stuff), with substantially more M2 orange: https://tbpl.mozilla.org/?tree=Try&rev=eb11d3369338 Oh, and checking just now I see a later M2 Fedora opt orange for the same reason: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d72db9163971 And given the oranging test in question was *added* by this patch, I think that's reasonably conclusive evidence that something's wrong *somewhere*, here. No idea why my try-push happened to stumble mightily enough on it for me to notice so strongly -- I'm pretty sure I didn't change anything near that code.

So the original orange (comment 42) was being caused by the "20 concurrent cursors" test overwriting the count variable, and can be fixed by using a closure to capture the value when creating the cursors. Incidentally, one of my pushes to diagnose that issue showed another orange in the "Test cache invalidation between getAll and getNext" test. In that case, |createResult1| was being used to delete what was supposed to be the last contact in the list, but since saving a contact is async, there were to guarantees that it was actually the last contact, or in the case of this particular orange, the contact that had already been returned in the first call to getAll(), causing the cursor to iterate through all 20 contacts instead of the expected 19. To fix it I get the last contact using a find({}) call and delete that one, which is guaranteed not to be returned by the cursor already. Interdiff from v8: http://pastebin.mozilla.org/2153986

Comment on attachment 715017 [details] [diff] [review] Implement nsIDOMContactManager.getAll(), v9 Review of attachment 715017 [details] [diff] [review]: ----------------------------------------------------------------- I just noticed that we sometimes use 2 transactions in our DB code for a single operation. We should try to remove this with a followup patch. Was the try run successful? ::: dom/contacts/fallback/ContactDB.jsm @@ +402,5 @@ > + if (cursor.value[i] == aObjectId) { > + if (DEBUG) debug("id matches cache"); > + cursor.value.splice(i, 1); > + cursor.update(cursor.value).onsuccess = function() { > + cursor.continue(); Revert the changes here. I don't think you need to call continue in the onsuccess function. They all happen after each other in the same transaction.

(In reply to Gregor Wagner [:gwagner] from comment #44) > Was the try run successful? Yes: https://tbpl.mozilla.org/?tree=Try&rev=ba67a04427f6 Comments addressed, r=gwagner.

(In reply to Reuben Morais [:reuben] from comment #20) > Time from getAll to first contact using the code in > https://github.com/reubenmorais/gaia/tree/dummy-contacts-cursor is: > > 1000 contacts, no cache: 1700ms > 1000 contacts, cache: 250ms New times with latest patch: 1000 contacts, no cache: 1132ms 1000 contacts, cache: 215ms

Ignore the timings above. I'm using the reference workloads that were added recently so others can test with the same data. reference-workload- light medium heavy x-heavy number of contacts 200 500 1000 2000 First contact: find 932ms 1635ms 2888ms 5306ms getAll (no cache) 652ms 1239ms 1812ms 3781ms getAll (cache) 180ms 186ms 189ms 201ms Render all contacts: find 2.9s 8.4s 19.9s 54.4s getAll (no cache) 12.5s 45.1s 84.5s 115.4s getAll (cache) 11.9s 44.9s 86.2s 135.0s This patch gives us a very good improvement for "time to first interaction". We get faster times without a cache, and very fast times - regardless of the number of contacts - when we do have a cache. On the other hand, rendering each contact at a time means it takes much longer to render the entire list, and the UI is less responsive during that time. This could be improved in the app by rendering in chunks (the code I'm using now is very simple, just to test the API), or if that doesn't help, by returning in chunks in the getAll API itself.

r=gwagner on IRC

(In reply to Reuben Morais [:reuben] from comment #50) > Ignore the timings above. I'm using the reference workloads that were added > recently so others can test with the same data. > > reference-workload- light medium heavy x-heavy > number of contacts 200 500 1000 2000 > > First contact: > find 932ms 1635ms 2888ms 5306ms > getAll (no cache) 652ms 1239ms 1812ms 3781ms > getAll (cache) 180ms 186ms 189ms 201ms > > Render all contacts: > find 2.9s 8.4s 19.9s 54.4s > getAll (no cache) 12.5s 45.1s 84.5s 115.4s > getAll (cache) 11.9s 44.9s 86.2s 135.0s > > > This patch gives us a very good improvement for "time to first interaction". > We get faster times without a cache, and very fast times - regardless of the > number of contacts - when we do have a cache. > > On the other hand, rendering each contact at a time means it takes much > longer to render the entire list, and the UI is less responsive during that > time. This could be improved in the app by rendering in chunks (the code I'm > using now is very simple, just to test the API), or if that doesn't help, by > returning in chunks in the getAll API itself. Yeah this needs some more work. Lets try to add chunks and some pre-fetching. When we call continue you can fetch contact[current + 2] from the parent and return the previous fetched contact[i+1]. Or maybe just move contacts in chunks from the parent to the child and the current approach might be fast enough.

Comment on attachment 715801 [details] [diff] [review] Follow-up, only release cursors in ContactService if they exist https://hg.mozilla.org/integration/mozilla-inbound/rev/69e5c650ee7f

This work is needed to meet a remaining performance goal for the contacts application.

Reuben, the b2g18 patch doesn't apply cleanly to b2g18 anymore. Can you post a new one please? Presumably with the follow-up included in it. Thanks!

(In reply to Ryan VanderMeulen [:RyanVM] from comment #56) > Reuben, the b2g18 patch doesn't apply cleanly to b2g18 anymore. Can you post > a new one please? Presumably with the follow-up included in it. Thanks! Sure, but bug 836513 and bug 837917 need to land before this.

Oh that's right *facepalm*. I forgot about those and leo? noms don't get triaged daily. I'll check back once those get approved and let you know if anything else is needed here.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4b962ed0f329 Pushed with the follow-up patch folded in.

Blocking tef+, therefore this is tef+.

Documented here: https://developer.mozilla.org/en-US/docs/Web/API/ContactManager.getAll