ES6 destructuring assignment performance too slow
Categories
(Core :: JavaScript Engine, enhancement, P3)
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 ?
Updated•9 years ago
|
Updated•9 years ago
|
Comment 1•9 years ago
|
||
(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.
Comment 3•9 years ago
|
||
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....
Comment 4•9 years ago
|
||
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.)
Comment 5•7 years ago
|
||
I now get numbers in the range of 0.6 to 4.0, that is probably enough to call this fixed.
Comment 6•5 years ago
|
||
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?
Comment 8•3 years ago
|
||
Sounds cool!
I think it'd be worth seeing the patch at the bare minimum :)
Comment 10•3 years ago
|
||
Hey Anba,
Would you be able to give some feedback on the attached patch for potentially speeding up destructuring?
Comment 11•3 years ago
|
||
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 theArray[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 toarray[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();
Updated•3 years ago
|
Comment 12•3 years ago
|
||
Three issues I've spotted right away
If those are not covered by test-262 tests, they should be...
Comment 13•3 years ago
|
||
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.
Comment 14•2 years ago
|
||
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.
Updated•11 months ago
|
Description
•