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

RESOLVED FIXED in mozilla29

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Mic, Assigned: jorendorff)

Tracking

Trunk
mozilla29
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
Created attachment 823654 [details] [diff] [review]
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. :)
Assignee: general → jorendorff
(Assignee)

Comment 3

5 years ago
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).
(Assignee)

Comment 4

5 years ago
Created attachment 823697 [details] [diff] [review]
bug-793860-destructuring-swap-v2.patch

Ends up being -23 lines net.
Attachment #823654 - Attachment is obsolete: true
Attachment #823697 - Flags: review?(luke)

Comment 5

5 years ago
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+
(Assignee)

Comment 6

4 years ago
Created attachment 8342020 [details] [diff] [review]
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.
Attachment #823697 - Attachment is obsolete: true
Attachment #8342020 - Flags: review?(luke)
(Assignee)

Comment 7

4 years ago
Created attachment 8342021 [details] [diff] [review]
Part 2 - Remove JSOP_ENUMELEM.
Attachment #8342021 - Flags: review?(luke)

Updated

4 years ago
Attachment #8342020 - Flags: review?(luke) → review+

Comment 8

4 years ago
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 :)
(Assignee)

Comment 10

4 years ago
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.
(Assignee)

Comment 11

4 years ago
Created attachment 8342751 [details] [diff] [review]
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.
Attachment #8342751 - Flags: review?(paul)
(Assignee)

Comment 12

4 years ago
Created attachment 8342757 [details] [diff] [review]
Part 0b - Declare some accidentally undeclared variables

(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+

Updated

4 years ago
Attachment #8342751 - Flags: review?(paul) → review+
(Assignee)

Comment 15

4 years ago
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.
(Assignee)

Comment 17

4 years ago
Created attachment 8344894 [details] [diff] [review]
Part 0c - Declare some more accidentally undeclared variables
Attachment #8344894 - Flags: review?(ahuang)
(Assignee)

Comment 18

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

Comment 19

4 years ago
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)

Updated

4 years ago
Attachment #8344894 - Flags: review?(ahuang) → review+
(Assignee)

Comment 22

4 years ago
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
Last Resolved: 4 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.