Open Bug 1165569 Opened 9 years ago Updated 11 months ago

ES6 destructuring assignment performance too slow

Categories

(Core :: JavaScript Engine, enhancement, P3)

38 Branch
enhancement

Tracking

()

People

(Reporter: movie1254v, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20150513174244

Steps to reproduce:

(1) Set up a simple test to test es6 destructuring assignment speed compared to the old assignment speed.
     Please see the setup test : http://fischer-l.github.io/testes6.html

(2) Load the test page (http://fischer-l.github.io/testes6.html) in browsers supporting es6 destructuring assignment.

(3) See the performance of the es6 desctructuring assignment and of the old assignment.


Actual results:

(1) In FF 38.0.1 :
     The es6 destructuring assignment could take 100ms on average for the test.
     The old assignment could take 4ms on average on average for the test.

(2) In Safari iOS 8.1 :
     The es6 destructuring assignment could take 13~20 ms on average for the test.
     The old assignment could take 15~22 ms on average for the test.


Expected results:

Compare to Safari iOS 8.1, the es6 destructuring assignment in FF 38.0.1 cost is so high.

Could the es6 destructuring assignment in FF be faster ?
Blocks: es6
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Severity: normal → major
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to fs from comment #0)
> Compare to Safari iOS 8.1, the es6 destructuring assignment in FF 38.0.1
> cost is so high.

Safari does not yet implement array destructuring correctly, therefore the performance difference between Safari and Firefox for the given test case is more or less irrelevant. If you change the following line to use object instead of array destructuring, it's possible to emulate Safari's current array destructuring behaviour:
> var { data : [ id, { name } ] } = user;

to:
> var { data : { 0: id, 1: { name } } } = user;

You'll notice Firefox actually outperforms Safari when that change is applied (*). 

Apart from that, bug 1165348 should help to improve array destructuring performance in Firefox.


(*) Let's totally ignore Safari's performance degradation when using object destructuring with indexed property keys.
Thanks.

Yes, trying more cases, it turns out that the speed of obj desctructuring is quite good, however the speed of array desctructuring is slow.
Just as a note, the "old_assign_to_array" and "es6_assign_to_array" functions in the test have different behavior depending on the exact state of "array", ArrayIterator.prototype, etc.  Per spec, array destructuring has to use the iteration protocol, which is pretty much designed to be as hard to optimize as possible.  So it's very unlikely that it can end up competitive with straight assignment from constant indices....
Depends on: 1165348
As the benchmark shows, object destructuring is already fast. With inlining and dead code elimination, the JIT would ideally optimize away the object destructuring cases here entirely, emptying the loops. I don't know why this isn't happening. If it did happen, we would see "0ms" for those tests.

Array destructuring speed depends on a particular optimization: scalar replacement. That's why bz linked this to bug 1165348.

I don't think we should try to optimize for the more unusual destructuring cases in this microbenchmark. But optimizing `for (var [k, v] of myMap) ...` would be a fine thing.

(Unblocking es6 because that meta-bug is about implementing the spec.)
No longer blocks: es6
I now get numbers in the range of 0.6 to 4.0, that is probably enough to call this fixed.
Destructuring assignment is SUPER slow, not practical to use whatsoever.

For example, if I want to rotate a matrix by pi/2, I would do:

[xx, yx, xy, yy] = [xy, yy, -xx, -yx];

Compared to swapping with a temp variable, the above line is:
- 10% faster in Chrome 70
- 99% slower in Firefox 63

https://jsperf.com/multi-swap

I have a working prototype that (mis?-)uses https://searchfox.org/mozilla-central/source/js/src/vm/Opcodes.h#1689 to do manual lookup when possible, bringing performance up to par with object destructuring.
Destructuring assignment from array literals seems to be more or less free in Chrome, but that seems like it would require a different optimization.

Is this prototype something that would be worth expanding into a real patch or is this a poor approach?

Sounds cool!

I think it'd be worth seeing the patch at the bare minimum :)

Hey Anba,

Would you be able to give some feedback on the attached patch for potentially speeding up destructuring?

Flags: needinfo?(andrebargull)

It's going to be a bit tricky to ensure the approach won't introduce spec compliance bugs. Three issues I've spotted right away:

  • It's not valid to call Array.prototype.slice, because that function reads the Array[Symbol.species], which isn't correct for array destructuring.
  • Out-of-bounds accesses to the input array must be detected. For example given var array = [0]; var [x, y] = array;, there mustn't be any access to array[1].
  • Iterator return calls mustn't be skipped. The expected behaviour for the following code snippet is that "set x: 1" is printed, followed by "return called". So when deciding if the fast-path version can be emitted, the assignment targets need to be examined and if the assignment can trigger side-effects, the fast-path can't be used:
function f() {
  var scope = {
    set x(value) {
      print("set x: ", value);

      Object.getPrototypeOf([].values()).return = function() {
        print("return called");
        return {};
      };

      throw new Error();
    }
  };

  var array = [1];

  with (scope) {
    // Looks like a variable declaration, but actually calls a setter on the with-object.
    var [x] = array;
  }
}
f();
Flags: needinfo?(andrebargull)
Attachment #9234281 - Attachment is obsolete: true

Three issues I've spotted right away

If those are not covered by test-262 tests, they should be...

I am so sorry about this, I completed overlooked that I hadn't run all the tests. If I'm doing it right, it looks like it passes test-262 (mach jstests test262 reports 40645 passed tests, 833 skipped, 0 failed) but it additionally fails non262/expressions/destructuring-array-lexical.js.
Thank you for taking the time to review this and set me straight on why this wouldn't work.

QA Whiteboard: qa-not-actionable

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Severity: major → --
Severity: -- → S3
Type: defect → enhancement
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: