793860 - Swapping variables with destructuring assignment is painfully slow (compared to using a temporary variable)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Benedikt Pfeifer]

Swapping variables using [x, y] = [y, x] is nearly two orders of magnitude slower than doing the same with a temporary variable. Maybe this is a common-enough use-case to speed it up somehow?
It's mentioned on MDN at least, so maybe a few people are really doing this: https://developer.mozilla.org/en-US/docs/JavaScript/New_in_JavaScript/1.7#Avoiding_temporary_variables . 


Performance tests:

** Destructuring assignment (snippet was run five times each on the error console):

  var startTime = Date.now(), xL = "L", xR = "R";
  for(var i=0; i < 5000000; i++) {
   [xL, xR] = [xR, xL];
  }
  var endTime = Date.now();
  endTime - startTime


Results (fastest and slowest run):
  Firefox 15.0.1: 3679ms to 3716ms
  Nightly 2012-09-24:  4714ms to 4919ms (a performance regression (~30% worse than 15.0.1) too?!) <-- this could need an own bug too?


**  Temporary variable (five times each again):

  var startTime = Date.now(), xL = "L", xR = "R";
  for(var i=0; i < 5000000; i++) {
   var tmp = xL;
   xL = xR;
   xR = tmp;
  }
  var endTime = Date.now();
  endTime - startTime


Results (fastest and slowest run):
  Firefox 15.0.1: 74ms to 91ms
  Nightly 2012-09-24: 79ms to 89ms

Comment 1

[Jason Orendorff [:jorendorff]]

First of all, note that if you put this code in a function, it is blisteringly fast, and equally fast with or without destructuring.

However, you're right, we're unnecessarily slow on this code when it appears in global code. Easy to fix, I think.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: WIP 1

With this, destructuring is even faster than using a tmp!

I think that is just because the tmp is a global variable, but I still think it's funny. :)

Comment 3

[Jason Orendorff [:jorendorff]]

However running that scrap of code in the Web Console takes 3 seconds for me. I don't know why. In a <script> tag it's fast (something like 300msec the first time, 17msec thereafter).

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: bug-793860-destructuring-swap-v2.patch

Ends up being -23 lines net.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 823697 [details] [diff] [review]
bug-793860-destructuring-swap-v2.patch

Nice!  But the hits don't stop there!  I think you've removed the only use of JSOP_ENUMCONSTELEM, so you can remove the opcode.

Also, there is only one remaining use of JSOP_ENUMELEM, and that's a few lines down in the PNK_DOT case.  Perhaps you could remove this too using swap?

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 1 - Simplify group assignment bytecode and remove JSOP_ENUMCONSTELEM, v3

Finally getting back to this after my "missing month" making modules.

This patch is the same as v2 except:

- the last hunk in BytecodeEmitter.cpp contains new code for the PNK_DOT case.

- the changes in jsopcode.tbl, Interpreter.cpp, and Xdr.h are new; these remove ENUMCONSTELEM, as suggested. They also get rid of JSOP_GETFUNNS, which is long obsolete.

Enough to warrant a quick look, I think.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 2 - Remove JSOP_ENUMELEM.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8342021 [details] [diff] [review]
Part 2 - Remove JSOP_ENUMELEM.

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

It's gettin' better all the tiiime

::: js/src/frontend/BytecodeEmitter.cpp
@@ +2830,5 @@
>      JS_ASSERT(emitOption != DefineVars);
>  
> +    // Now emit the lvalue opcode sequence. If the lvalue is a nested
> +    // destructuring initialiser-form, call ourselves to handle it, then pop
> +    // the matched value. Otherwise emit an lvalue bytecode sequence followed

Technically the pop only occurs if emitOption == InitializeVars.

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> - the changes in jsopcode.tbl, Interpreter.cpp, and Xdr.h are new; these
> remove ENUMCONSTELEM, as suggested.

Great, this is/was one of the few ops we interpret but don't baseline-compile :)

Comment 10

[Jason Orendorff [:jorendorff]]

Well, the first patch here fixes a bug.

    "use strict"; [a, b] = [1, 2];

Previously erroneously used JSOP_SETELEM and thus would create two new global properties. Now it correctly throws a runtime ReferenceError.

Surely this minor change doesn't break any existing code in the browser, though.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Part 0a - Declare an accidentally undeclared variable

Paul, I found a bug in the JS engine that affects this line observable-object.js:

    [desc.value, path] = this.unwrap(target, key, desc.value);

The bug is that we allow this. This should be a strict-mode ReferenceError, because path isn't declared and this file is "use strict".

The attached patch, r?you, simply declares path as a local, so that when we fix the JS engine bug to be stricter, this working code doesn't break.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 0b - Declare some accidentally undeclared variables

(That was a joke in comment 10, btw. I already knew I'd broken some stuff.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8342757 [details] [diff] [review]
Part 0b - Declare some accidentally undeclared variables

r=me

Comment 14

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e4bee000fd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c868d65aaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/c84b0156ae03
https://hg.mozilla.org/integration/mozilla-inbound/rev/fabe232e8be9

Comment 15

[Jason Orendorff [:jorendorff]]

The simplification and the correctness fix are welcome anyway; unfortunately we will probably regress a bit on performance when we change the semantics to be ES6 iteration.

http://people.mozilla.org/~jorendorff/es6-draft.html#sec-runtime-semantics-destructuringassignmentevaluation
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-runtime-semantics-iteratordestructuringassignmentevaluation

Then [a, b] = [b, a] will explode to a ton of bytecode. Recovering the lost speed is a tall order for the JITs.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Parts 1 and 2 backed out for Linux mochitest-2 timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9244495099db

https://tbpl.mozilla.org/php/getParsedLog.php?id=31699811&tree=Mozilla-Inbound

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: Part 0c - Declare some more accidentally undeclared variables

Comment 18

[Jason Orendorff [:jorendorff]]

I also filed bug 948134 with a patch, but I think it's just incidental.

Comment 19

[Alan Huang]

Comment on attachment 8344894 [details] [diff] [review]
Part 0c - Declare some more accidentally undeclared variables

I think this looks good to use them as block scope local variables. But I think I cannot review this, sorry. Forward review? to Kyle.

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3e4bee000fd2
https://hg.mozilla.org/mozilla-central/rev/f9c868d65aaf

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8344894 [details] [diff] [review]
Part 0c - Declare some more accidentally undeclared variables

I'm happy for Alan to review this.

Comment 22

[Jason Orendorff [:jorendorff]]

In part 0c, I had to switch from 'let' to 'var' because that test code loads as normal content JS, not chrome or version=1.8.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d468e098e39
https://hg.mozilla.org/integration/mozilla-inbound/rev/b44d4155293d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac98c7023f6

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d468e098e39
https://hg.mozilla.org/mozilla-central/rev/b44d4155293d
https://hg.mozilla.org/mozilla-central/rev/6ac98c7023f6