Benedikt Pfeifer Reporter
	Description • 12 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

	Jason Orendorff [:jorendorff] Assignee
	Comment 1 • 11 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 2 • 11 years ago

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. :)

	Jason Orendorff [:jorendorff] Assignee
	Comment 3 • 11 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).

	Jason Orendorff [:jorendorff] Assignee
	Comment 4 • 11 years ago

Ends up being -23 lines net.

	Luke Wagner [:luke]
	Comment 5 • 11 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?

	Jason Orendorff [:jorendorff] Assignee
	Comment 6 • 11 years ago

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.

	Luke Wagner [:luke]
	Comment 8 • 11 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.

	Jan de Mooij [:jandem]
	Comment 9 • 11 years ago

(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 :)

	Jason Orendorff [:jorendorff] Assignee
	Comment 10 • 11 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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 11 • 11 years ago

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.

	Jason Orendorff [:jorendorff] Assignee
	Comment 12 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

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

r=me

	Jason Orendorff [:jorendorff] Assignee
	Comment 14 • 11 years ago

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 15 • 11 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

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

	Jason Orendorff [:jorendorff] Assignee
	Comment 18 • 11 years ago

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

	Alan Huang
	Comment 19 • 11 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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 20 • 11 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 21 • 11 years ago

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

I'm happy for Alan to review this.

	Jason Orendorff [:jorendorff] Assignee
	Comment 22 • 11 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

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 11 years ago

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