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

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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

[Georgia Wang]

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

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: 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.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I wrote bug 927548 to track any work towards scalar replacement.

Comment 6

[Rick Waldron [:rwaldron]]

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

[Rick Waldron [:rwaldron]]

Relevant: https://bugzilla.mozilla.org/show_bug.cgi?id=874580

Comment 8

[Gregor Wagner [:gwagner]]

Comment on attachment 817998 [details] [diff] [review]
contacts-avoid-for-of

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

Thanks!

Comment 9

[Reuben Morais [:reuben]]

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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/330b19c29960

Comment 11

[David Bruant]

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

[David Bruant]

whoooaaa! No nevermind me. Sorry for the noise.

Comment 13

[Reuben Morais [:reuben]]

Yep, this code got _slightly_ more complicated with WebIDL :)

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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