Closed Bug 793860 Opened 7 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: Mic, Assigned: jorendorff)

Details

Attachments

(5 files, 2 obsolete files)

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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch WIP 1 (obsolete) — Splinter Review
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. :)
Assignee: general → 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).
Ends up being -23 lines net.
Attachment #823654 - Attachment is obsolete: true
Attachment #823697 - Flags: review?(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?
Attachment #823697 - Flags: review?(luke) → review+
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.
Attachment #823697 - Attachment is obsolete: true
Attachment #8342020 - Flags: review?(luke)
Attachment #8342021 - Flags: review?(luke)
Attachment #8342020 - Flags: review?(luke) → review+
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.
Attachment #8342021 - Flags: review?(luke) → review+
(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 :)
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.
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.
Attachment #8342751 - Flags: review?(paul)
(That was a joke in comment 10, btw. I already knew I'd broken some stuff.)
Attachment #8342757 - Flags: review?(bzbarsky)
Comment on attachment 8342757 [details] [diff] [review]
Part 0b - Declare some accidentally undeclared variables

r=me
Attachment #8342757 - Flags: review?(bzbarsky) → review+
Attachment #8342751 - Flags: review?(paul) → review+
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.
I also filed bug 948134 with a patch, but I think it's just incidental.
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.
Attachment #8344894 - Flags: review?(ahuang) → review?(khuey)
Comment on attachment 8344894 [details] [diff] [review]
Part 0c - Declare some more accidentally undeclared variables

I'm happy for Alan to review this.
Attachment #8344894 - Flags: review?(khuey) → review?(ahuang)
Attachment #8344894 - Flags: review?(ahuang) → review+
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
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/5d468e098e39
https://hg.mozilla.org/mozilla-central/rev/b44d4155293d
https://hg.mozilla.org/mozilla-central/rev/6ac98c7023f6
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.