844558 - Contact API: Send cached contacts in chunks from parent to child

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

Attachment: Send contacts in chunks

We can minimize the amount of round trips between child and parent processes by sending contacts in chunks.

This patch builds on top of the one in bug 844274.

Comment 1

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

Comment on attachment 717577 [details] [diff] [review]
Send contacts in chunks

Review of attachment 717577 [details] [diff] [review]:
-----------------------------------------------------------------

The downside with this approach is that we have to load 20 contacts from the database before we display the first one. So the time-to-first-result might be affected. Please check that that isn't the case.

r=me assuming that you've checked that.

::: dom/contacts/fallback/ContactDB.jsm
@@ +529,3 @@
>          if (aFullContacts) {
>            if (DEBUG) debug("full contacts: " + aCachedResults.length);
> +          for (let i = 0; i < ~~(aCachedResults.length/CHUNK_SIZE)+1; ++i) {

would be simpler as:

while (aCachedResults.length) {

@@ +544,5 @@
> +                  aSuccessCb(chunk);
> +                  chunk.length = 0;
> +                  count++;
> +                  if (count >= chunkCount) {
> +                    aSuccessCb(null);

To avoid the risk of rounding or off-by-one errors, it would be simpler to move this part outside of the |if (chunk.length == CHUNK_SIZE)| branch and do something like:

chunk.push(e.target.result);
count++;
if (count == aCachedResults.lenght) {
  aSuccessCb(chunk);
  aSuccessCb(null);
}
else if (chunk.length == CHUNK_SIZE) {
  aSuccessCb(chunk);
  chunk.length = 0;
}

Comment 2

[Reuben Morais [:reuben]]

(In reply to Jonas Sicking (:sicking) from comment #1)
> The downside with this approach is that we have to load 20 contacts from the
> database before we display the first one. So the time-to-first-result might
> be affected. Please check that that isn't the case.

The time to first contact is affected, but not a lot more than what bug 844274 already did (see bug 844274 comment 6 and compare it with bug 836519 comment 50). We get slower on time to first contact when there is a cache. Of course, for the next 19 contacts, the round trip time is 0, so "time to first 20 contacts" is much better with chunking.

An alternative to alleviate this would be to make the first chunk smaller or send the first contact(s) individually, but it's going to be an arbitrary number in the end, for a very minor gain. What if the next device is a tablet that can show 25 contacts at once? Will we change the chunk size again?

Comment 3

[Reuben Morais [:reuben]]

Attachment: Send contacts in chunks

Re-requesting review since this has significant changes. To minimize time to first chunk/contact, I spin the event loop between chunks, so I can start sending chunks earlier, as discussed on IRC. Interdiff at http://pastebin.mozilla.org/2188904

Comment 4

[Reuben Morais [:reuben]]

Attachment: Patch, rebased to mozilla-b2g18

Rebased to b2g18, where there is no Timer.jsm.

Comment 5

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e765f7f8f994

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e765f7f8f994

Comment 7

[Reuben Morais [:reuben]]

Attachment: Patch, rebased to mozilla-b2g18

The other one was missing a bind-this-chain :(

Comment 8

[Julien Wajsberg [:julienw]]

blocks tef+ bug 846895 -> asking for tef?

Reuben, could you make a performance table like in Bug 844274 comment 6 ?

Comment 9

[Michael Vines [:m1] [:evilmachines]]

(tef+, blocks a blocker)

Comment 10

[Reuben Morais [:reuben]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e7996a13db8b
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c8df1a66205c

Comment 11

[Reuben Morais [:reuben]]

ni? for the performance numbers.

Comment 12

[Reuben Morais [:reuben]]

reference-workload-     light     medium       heavy    x-heavy
number of contacts        200        500        1000       2000

First to first contact (chunk):
find                    932ms     1635ms      2888ms     5306ms
getAll (no cache)       488ms     1351ms      1790ms     4271ms
getAll (cache)          357ms      279ms       302ms      316ms

Render all contacts:
find                     2.9s       8.4s       19.9s      54.4s
getAll (no cache)        3.1s       6.4s       17.6s      23.4s
getAll (cache)           2.6s       7.5s       18.2s      26.8s

A few things that should be noted: time to first contact here means time to first chunk. The slightly longer time to first contact is amortized by the next 19 contacts being available immediately. Time to render all contacts here shouldn't be compared to the one in bug 844274 comment 6, because the Gaia tree I used to get the numbers in that bug had very minimal (and faster) rendering of the contacts.

Comment 13

[Reuben Morais [:reuben]]

Backed out for breaking Mochitests.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/bb5327d7e599
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/36d35b08e68a

Comment 14

[Reuben Morais [:reuben]]

Alright, let's try this again.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5aacf880400b
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/4931ec89ebbe

Comment 15

[Reuben Morais [:reuben]]

Follow up to fix a rebase mistake: https://hg.mozilla.org/releases/mozilla-b2g18/rev/ff9f164d0cff