Array destructuring slower than manual array lookup

NEW
Unassigned

Status

()

Core
JavaScript Engine
2 years ago
5 months ago

People

(Reporter: Kevin Decker, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

38 Branch
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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. git@github.com: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.
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
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?
Flags: needinfo?(jdemooij)
Created attachment 8626475 [details]
Testcase
> 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.
Depends on: 1165569

Updated

2 years ago
Blocks: 1177735
(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...
Depends on: 1038859
Clearing NI for now. I want to get to this soon.
Flags: needinfo?(jdemooij)

Updated

2 years ago
Depends on: 1204073
(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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
We are about 600x times slower on https://arewefastyet.com/#machine=29&view=single&suite=six-speed&subtest=destructuring-es6.
I just profiled this.  Now we're spending 99.6% of our time under intrinsic_NewArrayIterator.
We should obviously elide this allocation, I think Nicolas worked on this before.
Flags: needinfo?(nicolas.b.pierron)
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.
Flags: needinfo?(nicolas.b.pierron)
Depends on: 1352006
Depends on: 1353170
You need to log in before you can comment on or make changes to this bug.