836519 - Contact API: Add a nsIDOMContactManager.getAll method that uses cursors

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Reuben Morais [:reuben]]

Attachment: WIP, Implement nsIDOMContactManager.getAll()

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.

Comment 2

[Reuben Morais [:reuben]]

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;
>  },

Comment 3

[Reuben Morais [:reuben]]

Attachment: WIP, Implement nsIDOMContactManager.getAll()

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

Comment 4

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll()

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.

Comment 5

[Alex Keybl [:akeybl]]

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

Comment 6

[alberto.pastor]

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

Thanks.

Comment 7

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

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.

Comment 8

[Reuben Morais [:reuben]]

Attachment: nsIDOMContactManager.getAll(), rebased to mozilla-b2g18

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

Comment 9

[alberto.pastor]

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!

Comment 10

[Reuben Morais [:reuben]]

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

Comment 11

[alberto.pastor]

As we talked offline, I left my tests in

https://github.com/albertopq/gaia/tree/dummy-contacts-cursor

Waiting for your feedback. Thanks!

Comment 12

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll()

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

Comment 13

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll()

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

Comment 14

[Reuben Morais [:reuben]]

Attachment: This patch and the ones it depends on, lumped together and rebased to mozilla-b2g18

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

Comment 15

[alberto.pastor]

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?

Comment 16

[Reuben Morais [:reuben]]

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

Comment 17

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v2

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 18

[Gregor Wagner [:gwagner]]

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

Comment 19

[Reuben Morais [:reuben]]

Attachment: Patches from bug 836513, bug 837917 and this bug, folded and rebased to mozilla-b2g18

Comment 20

[Reuben Morais [:reuben]]

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

Comment 21

[Gregor Wagner [:gwagner]]

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

Comment 22

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v3

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 23

[Gregor Wagner [:gwagner]]

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]?

Comment 24

[Reuben Morais [:reuben]]

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

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 26

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 27

[Gregor Wagner [:gwagner]]

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

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 29

[Lucas Adamski [:ladamski]]

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.

Comment 30

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v4

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

Comment 31

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v4

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

Comment 32

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v5

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

Comment 33

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v6

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

Comment 34

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v7

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

Comment 35

[Reuben Morais [:reuben]]

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

Comment 36

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v8

Yet Another Version with changes discussed on IRC.

Comment 37

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v8

Sigh.

Comment 38

[Gregor Wagner [:gwagner]]

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

Comment 39

[Gregor Wagner [:gwagner]]

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.

Comment 40

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v8

Comments addressed, r=gwagner.

Comment 41

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a145f17e37c

Comment 42

[Jeff Walden [:Waldo]]

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.

Comment 43

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v9

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 44

[Gregor Wagner [:gwagner]]

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.

Comment 45

[Reuben Morais [:reuben]]

Attachment: Implement nsIDOMContactManager.getAll(), v9

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

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00623df83261

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00623df83261

Comment 48

[Reuben Morais [:reuben]]

Attachment: Patch, rebased to mozilla-b2g18

Comment 49

[Reuben Morais [:reuben]]

(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

Comment 50

[Reuben Morais [:reuben]]

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.

Comment 51

[Reuben Morais [:reuben]]

Attachment: Follow-up, only release cursors in ContactService if they exist

r=gwagner on IRC

Comment 52

[Gregor Wagner [:gwagner]]

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

[Ryan VanderMeulen [:RyanVM]]

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

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69e5c650ee7f

Comment 55

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 56

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 57

[Reuben Morais [:reuben]]

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

Comment 58

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4b962ed0f329

Pushed with the follow-up patch folded in.

Comment 60

[Reuben Morais [:reuben]]

Attachment: Patch, rebased to mozilla-b2g18(_v1_0_1)

Comment 61

[Alex Keybl [:akeybl]]

Blocking tef+, therefore this is tef+.

Comment 62

[Reuben Morais [:reuben]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f96bde368aa4

Comment 63

[Jeremie Patonnier :Jeremie]

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