mozContacts.getAll() time regressed due to es6 for-of iterator changes

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

({perf, regression})

Trunk
mozilla27
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c= p=3 s= u=])

Attachments

(1 attachment)

As discussed in bug 927028 we recently saw a regression in the time it takes mozContacts.getAll() to return 1000 contacts.  Bisecting shows that the majority of the slowdown was introduced in rev 149789:a4f96de49668.
Note, I was using running the following app on my b2g buri device:

  https://github.com/wanderview/gaia/tree/contacts-api-bench/test_apps/contacts-bench

Using 1000 contacts installed using gaia's 'make reference-workload-heavy' command.

Full times at different gecko revs can be seen in bug 927028 comment 1.
Profile of contacts API bench app at 149788:b7c5cd333e9c:

  http://people.mozilla.org/~bgirard/cleopatra/#report=cf48ead36de48aa6e5a49c88c2fe9b6efbc6fbcd

Profile of contacts API bench app at 149789:a4f96de49668:

  http://people.mozilla.org/~bgirard/cleopatra/#report=1d125e54acf127c8779c4c532f1070dcbe81b669&search=iterator

(I apologize for the spikes in the profiles.  I believe that is a bug in the native stack unwinding support for b2g.)

Filtering on "iterator" I don't see much in the parent process.  In the child process, however, it appears to hit the ContactsManager.jsm's _convertContacts() function by way of the for-of loop in validateArrayField():

  http://mxr.mozilla.org/mozilla-central/source/dom/contacts/ContactManager.js#222

Within validateArrayField() it appears the stack calling LegacyIteratorNext(), IteratorResult(), and ultimately malloc() consume the most time.

Does any of this make sense or help?  I'm not familiar enough with the code here to know what is expected or not.  Please let me know if I can provide any additional information.

Thanks!

Comment 3

6 years ago
I found that in Gaia pull-request, many people met contact-test timeout the two days.

https://travis-ci.org/mozilla-b2g/gaia/builds


For example:

 1) [communications-contacts] Matching duplicate contacts UI Test Suite duplicate contact details should show the duplicate contact name and highlight it:
Error: timeout of 2000ms exceeded
at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3680)

Not sure if this related to the bug..Just provide more information about this :)
There was some discussion over in bug 907077 about this problem.  To summarize:

 1) New es6 syntax requires a new object per iteration.
 2) This allocation is currently slow.
 3) Future optimizations, such as scalar replacement, should negative this time penalty.

So at the moment the for-of syntax is going through a transition and is a bit on the bleeding edge.  Until its fully optimized I think it makes sense for code like mozContacts to simply use more traditional loop syntax.

This patch removes the for-of from ContactManager.js and replaces it with a standard for loop.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #817998 - Flags: review?(anygregor)
I wrote bug 927548 to track any work towards scalar replacement.
Whiteboard: [c= p= s= u=] → [c= p=3 s= u=]
(In reply to Ben Kelly [:bkelly] from comment #4)
> Created attachment 817998 [details] [diff] [review]
> contacts-avoid-for-of
> 
> There was some discussion over in bug 907077 about this problem.  To
> summarize:
> 
>  1) New es6 syntax requires a new object per iteration.
>  2) This allocation is currently slow.
>  3) Future optimizations, such as scalar replacement, should negative this
> time penalty.
> 
> So at the moment the for-of syntax is going through a transition and is a
> bit on the bleeding edge.  Until its fully optimized I think it makes sense
> for code like mozContacts to simply use more traditional loop syntax.
> 
> This patch removes the for-of from ContactManager.js and replaces it with a
> standard for loop.

Just curious, does this code run in a version=1.8 environment?

Also, I think this is a reasonable side-by-side comparison of for-of vs. for-loop where they should be about the same, but aren't: http://jsperf.com/for-of-vs-for-loop

(The measured value is not important, only that the two forms should closely match)
Comment on attachment 817998 [details] [diff] [review]
contacts-avoid-for-of

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

Thanks!
Attachment #817998 - Flags: review?(anygregor) → review+
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/330b19c29960
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 11

6 years ago
Sorry for arriving after the battle, but it looks like this code could be shortened a bit to:

function validateArrayField(data, createCb) {
  if (data) {
    data = Array.isArray(data) ? data : [data];
    return data.filter(function(obj){
      return obj && isVanillaObj(obj);
    })
  else{
    return undefined;
  }
}

And if arrow functions are allowed:

return data.filter( obj => obj && isVanillaObj(obj) )

It might be faster than a bunch of calls .push() but maybe I'm wrong.

Comment 12

6 years ago
whoooaaa! No nevermind me. Sorry for the noise.
Yep, this code got _slightly_ more complicated with WebIDL :)
You need to log in before you can comment on or make changes to this bug.