Last Comment Bug 1177319 - Array destructuring slower than manual array lookup
: Array destructuring slower than manual array lookup
Status: NEW
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 38 Branch
: Unspecified Unspecified
-- normal with 4 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 1165569 1038859 1204073
Blocks: six-speed
  Show dependency treegraph
 
Reported: 2015-06-24 20:07 PDT by Kevin Decker
Modified: 2015-09-11 12:48 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (540 bytes, text/html)
2015-06-26 02:11 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details

Description User image Kevin Decker 2015-06-24 20:07:51 PDT
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.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2015-06-26 02:11:14 PDT
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 User image Boris Zbarsky [:bz] (still a bit busy) 2015-06-26 02:11:46 PDT
Created attachment 8626475 [details]
Testcase
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2015-06-26 02:17:50 PDT
> 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 User image Jan de Mooij [:jandem] 2015-06-27 09:16:09 PDT
(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 User image Jan de Mooij [:jandem] 2015-07-28 04:16:13 PDT
Clearing NI for now. I want to get to this soon.
Comment 6 User image Jan de Mooij [:jandem] 2015-09-11 12:48:22 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.