Closed Bug 855015 Opened 7 years ago Closed 7 years ago

Contacts API: Send chunks to child less often

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
1.0.1 Madrid (19apr)
blocking-b2g tef+
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: reuben, Assigned: reuben)

References

Details

(Whiteboard: [madrid][fixed-in-birch])

Attachments

(2 files, 10 obsolete files)

Attached patch Send chunks less often (obsolete) — Splinter Review
Right now we send the results to the child process as fast as we can, but that causes the Contacts app to be terribly unresponsive until all the contacts are sent. We should send the chunks less often, giving time for the app to do whatever processing it needs to do on the contacts, but send them quickly if the child's cache is empty.
Ah doesn't apply on b2g18. Can you merge it please? :)
Attached patch Patch, rebased to mozilla-b2g18 (obsolete) — Splinter Review
Attached patch Patch, rebased to mozilla-b2g18 (obsolete) — Splinter Review
I copied the contents of Timer.jsm over to the file just so we can test it. Ideally we'd uplift it to b2g18.
Attachment #730363 - Attachment is obsolete: true
Depends on: 840360
blocking-b2g: --- → tef?
Attached patch Send chunks less often (obsolete) — Splinter Review
Attachment #729717 - Attachment is obsolete: true
Attachment #730365 - Attachment is obsolete: true
Attachment #730803 - Flags: review?(anygregor)
Blocks: 855556
It is still not clear to me what the performance improvement is, we need those figures to understand whether we should block on this one.
just realized it blocks a blocker
blocking-b2g: tef? → tef+
Comment on attachment 730803 [details] [diff] [review]
Send chunks less often

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

::: dom/contacts/ContactManager.js
@@ +653,5 @@
>        this.nextTick(this._fireSuccessOrDone.bind(this, data.cursor, contact));
>      } else {
>        if (DEBUG) debug("waiting for contact");
>        data.waitingForNext = true;
> +      cpmm.sendAsyncMessage("Contacts:GetAll:SendNow", { cursorId: aCursorId });

We only do this if our cache is empty? Can't we send this earlier like when we only have 5 contacts left?
needinfo on Reuban for comment 7
Flags: needinfo?(reuben.bmo)
We're still discussing whether this will be needed to meet the performance requirements. As for the question, yes, we can. Whether that's useful or not is arguable since this is intended to improve scrolling/responsiveness.
Flags: needinfo?(reuben.bmo)
Who is discussing this, and where? We need to determine whether this blocks and whether it'll fix bug 848188 as soon as possible. What needs to happen to answer this question?
(In reply to Dietrich Ayala (:dietrich) from comment #10)
> Who is discussing this, and where? We need to determine whether this blocks
> and whether it'll fix bug 848188 as soon as possible. What needs to happen
> to answer this question?

Initial experiments by Alberto and Francisco to implement bug 848188 showed that scrolling is very janky during the initial loading of contacts, for several reasons. We initially thought the biggest culprit was the Contacts API sending chunks from the parent and using all the CPU — and that's why this bug exists —, but we're seeing a lot of sync reflows in the app, and we think changing the markup to minimize reflows will be enough to fix the scrolling problem. gwagner, bent, gaye and I met yesterday to understand the Contacts app and discuss how to improve the markup. 

This patch fixes the problem in an ugly way, and I think we should not land it unless it's absolutely necessary.
If the markup and reflows are the problem, why just rendering 1 chunk (just 1 reflow) and keep asking for more contacts blocks the UI until we have all the contacts?
(In reply to Alberto Pastor [:albertopq] from comment #12)
> If the markup and reflows are the problem, why just rendering 1 chunk (just
> 1 reflow) and keep asking for more contacts blocks the UI until we have all
> the contacts?

I believe that was before bug 851741 was fixed? Is that still the case?
I tried with the patch before landing and it was, but let me try again tomorrow.

Thanks!
(In reply to Alberto Pastor [:albertopq] from comment #14)
> I tried with the patch before landing and it was, but let me try again
> tomorrow.

Did you get a chance to try, Alberto?
Yep! Reuben is right, now is working properly. Check Bug 846895 for more info :)
err, I meant Bug 848188
(In reply to Alberto Pastor [:albertopq] from comment #16)
> Yep! Reuben is right, now is working properly. Check Bug 846895 for more
> info :)

So should we land this patch?
Nop, I tested it with bug 851741 and I think is enough. I think Reuben meant not landing this if the other one is enough.
I think this would be another improvement especially when we start scrolling at the beginning. Reuben do you want to profile?
(In reply to Gregor Wagner [:gwagner] from comment #20)
> I think this would be another improvement especially when we start scrolling
> at the beginning. Reuben do you want to profile?

Sure, but I doubt I'll get any useful information from profiling, if it's fast enough like Alberto said. I'll test with and without the patch and see how much it improves scrolling.
Based on the above comments I'd say we should not block on this one unless it is really needed.
blocking-b2g: tef+ → -
Attached patch Send chunks less often (obsolete) — Splinter Review
Rebased.
Attachment #730803 - Attachment is obsolete: true
Attachment #730803 - Flags: review?(anygregor)
Attachment #737885 - Flags: review?(anygregor)
Attached patch Send chunks less often (obsolete) — Splinter Review
There we go.
Attachment #737885 - Attachment is obsolete: true
Attachment #737885 - Flags: review?(anygregor)
Attachment #737924 - Flags: review?(anygregor)
I got some numbers for this patch. I measure the time from calling "mozContacts.getAll(options);" until we render the first contacts (within "cursor.onsuccess")

With this patch:
display first contact: 248msec
loading done: 7984msec

without this patch:
display first contact: 3224msec
loading done: 6047msec

This is a huge win for displaying the first contacts.
We need 2 seconds longer to load all contacts but we have plans to optimize this if we need it.
blocking-b2g: - → tef?
No longer blocks: 848188
triage: tef+ for contacts performance
blocking-b2g: tef? → tef+
This one has the small change discussed on IRC, we don't wait until the child is out of contacts before sending more.
Attachment #737924 - Attachment is obsolete: true
Attachment #737924 - Flags: review?(anygregor)
Attachment #738374 - Flags: review?(anygregor)
Attached patch Timer.jsm for b2g18 (obsolete) — Splinter Review
Attached patch Patch, rebased to mozilla-b2g18 (obsolete) — Splinter Review
Whiteboard: [madrid]
Attached patch Timer.jsm for b2g18 (obsolete) — Splinter Review
Fixing rebase hell.
Attachment #738384 - Attachment is obsolete: true
Comment on attachment 738374 [details] [diff] [review]
Send chunks less often

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

::: dom/contacts/tests/test_contacts_getall.html
@@ +385,5 @@
>              count++;
>              req.continue();
>            } else {
> +            checkContacts(result, properties2);
> +            is(count, 40, "cursor " + i + " returned 20 contacts");

"returned 40 contacts".
Maybe lets add 40 contacts and give them unique values we can check?
so contact 1 can have a name followed by 1 and we can check that we get the correct contact for every i
Attachment #738374 - Flags: review?(anygregor) → review+
Attached patch Patch, rebased to mozilla-b2g18 (obsolete) — Splinter Review
Attachment #738385 - Attachment is obsolete: true
Attachment #738444 - Attachment is obsolete: true
https://hg.mozilla.org/projects/birch/rev/14db91e6346f
Whiteboard: [madrid] → [madrid][fixed-in-birch]
Final rebased patch.
Attachment #738447 - Attachment is obsolete: true
Attachment #738482 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/14db91e6346f
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
No longer blocks: 855556
(In reply to Ryan VanderMeulen [:RyanVM] from comment #37)

Please let me know the next step for this issue.
Thanks.

> Backed out for mochitest-2 failures.
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/a5a89b7510cc
> https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5e5127529f5b
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=21922189&tree=Mozilla-
> B2g18_v1_0_1
Target Milestone: mozilla23 → Madrid (19apr)
Depends on: 867043
You need to log in before you can comment on or make changes to this bug.