1177319 - Array destructuring slower than manual array lookup

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Kevin Decker]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.130 Safari/537.36

Steps to reproduce:

1. [e-mail]:kpdecker/six-speed.git
2. npm install
3. gulp server
4. open http://localhost:9999/#destruct


Actual results:

Array destructuring was 200x slower than equivalent operations that manually declared and assigned variables using array lookups.

See destructuring section of http://kpdecker.github.io/six-speed/


Expected results:

Performance should be similar to the ES5 implementation.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Per spec, destructuring and reading indexed properties from an array aren't actually equivalent: destructuring has to iterate the iterator one gets from an array.  So chances are making destructuring just as fast as reading indexed properties in all cases is not possible; there are some cases in which they need to have quite different behavior and the destructuring just needs to do more work.

In particular if I'm correctly reading the testcase, you're comparing https://github.com/kpdecker/six-speed/blob/master/tests/destructuring/destructuring.es5 to https://github.com/kpdecker/six-speed/blob/master/tests/destructuring/destructuring.es6 right?  These tests are very comparing apples to oranges (e.g. the destructuring one examines two elements of the array, because it's iterating, while the non-destructuring one only examines one element).

It looks like Safari has about the same performance for destructuring and the array indexing... but that's at least in part because their destructuring implementation just doesn't follow the spec.  Simple testcase:

  var y = "FAIL";
  Object.defineProperty(Array.prototype, 0, { get: function() { y = "PASS"; } });
  var [,x] = [,1];
  alert(y);

this should alert "PASS" but alerts "FAIL" in Safari.

Anyway, assuming those are the two tests involved, I'll attach a simple testcase that lets one run the code in question.  That testcase shows that the time in the destructuring case is spent in 3 places, all about evenly split:

1)  Compiled jitcode, doing the various function calls that array destructuring requires.
2)  Creation of ArrayIterator objects.
3)  GetElementIC::update.

This last looks like the Symbol.iterator get, since it involves things like ToSymbolPrimitive.  Jan, any idea why we're hitting update() so much here?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

> It looks like Safari has about the same performance 

I just tried a WebKit nightly and they've fixed their spec bug.  Now my testcase alerts "PASS"... and the destructuring testcase is now 2x slower than in Firefox.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> This last looks like the Symbol.iterator get, since it involves things like
> ToSymbolPrimitive.  Jan, any idea why we're hitting update() so much here?

The bytecode looks like this:

00061:  getprop "arr"
00066:  dup
00067:  dup
00068:  symbol 0
00070:  callelem

JSOP_SYMBOL pushes a constant, so the CALLELEM is very similar to a GETPROP and we should fix IonBuilder to use TI to nop this or do something smarter. We also need GETELEM/SETELEM IC stubs...

Comment 5

[Jan de Mooij [:jandem]]

Clearing NI for now. I want to get to this soon.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> 1)  Compiled jitcode, doing the various function calls that array
> destructuring requires.
> 2)  Creation of ArrayIterator objects.
> 3)  GetElementIC::update.
> 
> This last looks like the Symbol.iterator get, since it involves things like
> ToSymbolPrimitive.  Jan, any idea why we're hitting update() so much here?

The patch in bug 1204073 gets rid of (3). After that we spend ~70% under intrinsic_NewArrayIterator, we should inline that probably at some point.

Comment 7

[Tom S [:evilpie]]

We are about 600x times slower on https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=destructuring-es6.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I just profiled this.  Now we're spending 99.6% of our time under intrinsic_NewArrayIterator.

Comment 9

[Tom S [:evilpie]]

We should obviously elide this allocation, I think Nicolas worked on this before.

Comment 10

[Nicolas B. Pierron [:nbp]]

IONFLAGS=eaa should report why the object allocation is not removed, but this suppose that we are running in Ion.

I will also note that we should first inline all function calls to be able to remove the allocation, which would not be the case in eager compilation of small functions.  In which case the recompilationCheck should trigger later to inline the intrinsic functions and remove the object allocation.