Closed Bug 844558 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

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

People

(Reporter: reuben, Assigned: reuben)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Send contacts in chunks (obsolete) — Splinter Review
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.
Attachment #717577 - Flags: review?(jonas)
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;
}
Attachment #717577 - Flags: review?(jonas) → review+
(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?
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
Attachment #717577 - Attachment is obsolete: true
Attachment #720255 - Flags: review?(jonas)
Attachment #720255 - Flags: review?(jonas) → review+
Attached patch Patch, rebased to mozilla-b2g18 (obsolete) — Splinter Review
Rebased to b2g18, where there is no Timer.jsm.
https://hg.mozilla.org/mozilla-central/rev/e765f7f8f994
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
The other one was missing a bind-this-chain :(
Attachment #720947 - Attachment is obsolete: true
blocks tef+ bug 846895 -> asking for tef?

Reuben, could you make a performance table like in Bug 844274 comment 6 ?
blocking-b2g: --- → tef?
(tef+, blocks a blocker)
blocking-b2g: tef? → tef+
ni? for the performance numbers.
Flags: needinfo?(reuben.bmo)
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.
Flags: needinfo?(reuben.bmo)
Blocks: 855015
You need to log in before you can comment on or make changes to this bug.