Skip to content

Commit

Permalink
[JSC] Remove ArrayPatternNode::emitDirectBinding()
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=187085
<rdar://problem/121959952>

Reviewed by Yusuke Suzuki.

This patch removes special bytecode generation path for code like `[a, b] = [b, a]` that wasn't
spec-compliant: it skipped invocation of iterator protocol, which was observable when built-ins like
`Array.prototype[Symbol.iterator]` were modified.

There is no way to guard the special code path with some sort of built-ins check without bloating
the bytecode size: we would still need to emit regular iterator protocol code, which actually
isn't even slower after DFG / FTL, as demonstated by existing microbenchmark.

Only very obscure / synthetic tests are recorded to be slower after this change.
This patch was A/B tested not to affect JS3, SP2, and SP3.
Aligns JSC with the spec, V8, and SpiderMonkey.

                                              ToT                     patch

destructuring-swap                      53.4546+-0.1895     ^     51.3538+-0.2101        ^ definitely 1.0409x faster
default-value-destructuring-array       36.5455+-0.4078     !    105.0089+-0.4452        ! definitely 2.8734x slower

* JSTests/microbenchmarks/default-value-destructuring-array.js:
* JSTests/microbenchmarks/destructuring-array-literal.js:
Removed, it was very synthetic test targeting emitDirectBinding().

* JSTests/microbenchmarks/destructuring-swap.js:
* JSTests/test262/expectations.yaml: Mark 24 tests as passing.
* Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:
(JSC::DestructuringAssignmentNode::emitBytecode):
(JSC::ArrayPatternNode::emitDirectBinding): Deleted.
* Source/JavaScriptCore/parser/Nodes.h:
(JSC::DestructuringPatternNode::isRestParameter const):
(JSC::DestructuringPatternNode::emitDirectBinding): Deleted.

Canonical link: https://commits.webkit.org/275474@main
  • Loading branch information
Alexey Shvayka committed Feb 29, 2024
1 parent 3ab87dc commit 7681169
Show file tree
Hide file tree
Showing 6 changed files with 5 additions and 90 deletions.
4 changes: 3 additions & 1 deletion JSTests/microbenchmarks/default-value-destructuring-array.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
(function() {
var o = {};
var a = [];
var s = "str";

for (var i = 0; i < 1e5; ++i) {
for (var i = 0; i < 2e5; ++i) {
var [
k00 = 0, k01 = 1, k02 = 2, k03 = 3, k04 = 4, k05 = 5, k06 = 6, k07 = 7, k08 = 8, k09 = 9,
k10 = 0, k11 = 1, k12 = 2, k13 = 3, k14 = 4, k15 = 5, k16 = 6, k17 = 7, k18 = 8, k19 = 9,
Expand All @@ -21,3 +22,4 @@ for (var i = 0; i < 1e5; ++i) {
o, a, s, o, a, s, o, a, s, o, a, s, o, a, s, o, o, s, o, a, s, o, o, s, o
];
}
})();
5 changes: 0 additions & 5 deletions JSTests/microbenchmarks/destructuring-array-literal.js

This file was deleted.

4 changes: 2 additions & 2 deletions JSTests/microbenchmarks/destructuring-swap.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ function foo(a, b) {
}

var result = 0;
for (var i = 0; i < 1000000; ++i)
for (var i = 0; i < 1e7; ++i)
result += foo(42, i);

if (result != 499957500000)
if (result != 49999575000000)
throw "Bad result: " + result;
36 changes: 0 additions & 36 deletions JSTests/test262/expectations.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1105,12 +1105,6 @@ test/language/statements/class/elements/privatefieldset-evaluation-order-1.js:
test/language/statements/class/subclass/default-constructor-spread-override.js:
default: 'Test262Error: @@iterator invoked'
strict mode: 'Test262Error: @@iterator invoked'
test/language/statements/const/dstr/ary-init-iter-get-err-array-prototype.js:
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
test/language/statements/const/dstr/ary-ptrn-elem-id-iter-val-array-prototype.js:
default: 'Test262Error: Expected SameValue(«3», «42») to be true'
strict mode: 'Test262Error: Expected SameValue(«3», «42») to be true'
test/language/statements/for-await-of/head-lhs-async.js:
default: "SyntaxError: Unexpected identifier 'of'"
strict mode: "SyntaxError: Unexpected identifier 'of'"
Expand Down Expand Up @@ -1181,35 +1175,5 @@ test/language/statements/for-of/dstr/array-rest-iter-thrw-close.js:
test/language/statements/for-of/dstr/array-rest-lref-err.js:
default: 'Test262Error: Expected SameValue(«1», «0») to be true'
strict mode: 'Test262Error: Expected SameValue(«1», «0») to be true'
test/language/statements/for/dstr/const-ary-init-iter-get-err-array-prototype.js:
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
test/language/statements/for/dstr/const-ary-ptrn-elem-id-iter-val-array-prototype.js:
default: 'Test262Error: Expected SameValue(«3», «42») to be true'
strict mode: 'Test262Error: Expected SameValue(«3», «42») to be true'
test/language/statements/for/dstr/let-ary-init-iter-get-err-array-prototype.js:
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
test/language/statements/for/dstr/let-ary-ptrn-elem-id-iter-val-array-prototype.js:
default: 'Test262Error: Expected SameValue(«3», «42») to be true'
strict mode: 'Test262Error: Expected SameValue(«3», «42») to be true'
test/language/statements/for/dstr/var-ary-init-iter-get-err-array-prototype.js:
default: 'Test262Error: Expected a TypeError but got a ReferenceError'
strict mode: 'Test262Error: Expected a TypeError but got a ReferenceError'
test/language/statements/for/dstr/var-ary-ptrn-elem-id-iter-val-array-prototype.js:
default: 'Test262Error: Expected SameValue(«3», «42») to be true'
strict mode: 'Test262Error: Expected SameValue(«3», «42») to be true'
test/language/statements/for/head-lhs-let.js:
default: "SyntaxError: Unexpected token ';'. Expected a parameter pattern or a ')' in parameter list."
test/language/statements/let/dstr/ary-init-iter-get-err-array-prototype.js:
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
test/language/statements/let/dstr/ary-ptrn-elem-id-iter-val-array-prototype.js:
default: 'Test262Error: Expected SameValue(«3», «42») to be true'
strict mode: 'Test262Error: Expected SameValue(«3», «42») to be true'
test/language/statements/variable/dstr/ary-init-iter-get-err-array-prototype.js:
default: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
strict mode: 'Test262Error: Expected a TypeError to be thrown but no exception was thrown at all'
test/language/statements/variable/dstr/ary-ptrn-elem-id-iter-val-array-prototype.js:
default: 'Test262Error: Expected SameValue(«3», «42») to be true'
strict mode: 'Test262Error: Expected SameValue(«3», «42») to be true'
44 changes: 0 additions & 44 deletions Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5445,8 +5445,6 @@ void ExportNamedDeclarationNode::emitBytecode(BytecodeGenerator&, RegisterID*)
// ------------------------------ DestructuringAssignmentNode -----------------
RegisterID* DestructuringAssignmentNode::emitBytecode(BytecodeGenerator& generator, RegisterID* dst)
{
if (RegisterID* result = m_bindings->emitDirectBinding(generator, dst, m_initializer))
return result;
RefPtr<RegisterID> initializer = generator.tempDestination(dst);
generator.emitNode(initializer.get(), m_initializer);
m_bindings->bindValue(generator, initializer.get());
Expand Down Expand Up @@ -5549,48 +5547,6 @@ void ArrayPatternNode::bindValue(BytecodeGenerator& generator, RegisterID* rhs)
generator.emitLabel(iteratorClosed.get());
}

RegisterID* ArrayPatternNode::emitDirectBinding(BytecodeGenerator& generator, RegisterID* dst, ExpressionNode* rhs)
{
if (!rhs->isSimpleArray())
return nullptr;

if (m_targetPatterns.findIf([&] (auto& target) { return target.bindingType == BindingType::RestElement; }) != notFound)
return nullptr;

ElementNode* elementNodes = static_cast<ArrayNode*>(rhs)->elements();
Vector<ExpressionNode*> elements;
for (; elementNodes; elementNodes = elementNodes->next()) {
ExpressionNode* value = elementNodes->value();
ASSERT(!value->isSpreadExpression());
elements.append(value);
}

RefPtr<RegisterID> resultRegister;
if (dst != generator.ignoredResult())
resultRegister = generator.emitNewArray(generator.newTemporary(), nullptr, 0, ArrayWithUndecided);
if (m_targetPatterns.size() != elements.size())
return nullptr;
Vector<RefPtr<RegisterID>> registers(m_targetPatterns.size(), [&](size_t i) {
RefPtr newRegister = generator.newTemporary();
generator.emitNode(newRegister.get(), elements[i]);
if (m_targetPatterns[i].defaultValue)
assignDefaultValueIfUndefined(generator, newRegister.get(), m_targetPatterns[i].defaultValue);
if (resultRegister) {
RefPtr index = generator.emitLoad(nullptr, jsNumber(i));
generator.emitDirectPutByVal(resultRegister.get(), index.get(), newRegister.get());
}
return newRegister;
});

for (size_t i = 0; i < m_targetPatterns.size(); i++) {
if (m_targetPatterns[i].pattern)
m_targetPatterns[i].pattern->bindValue(generator, registers[i].get());
}
if (resultRegister)
return generator.move(generator.finalDestination(dst, resultRegister.get()), resultRegister.get());
return generator.emitLoad(generator.finalDestination(dst), jsUndefined());
}

void ArrayPatternNode::toString(StringBuilder& builder) const
{
builder.append('[');
Expand Down
2 changes: 0 additions & 2 deletions Source/JavaScriptCore/parser/Nodes.h
Original file line number Diff line number Diff line change
Expand Up @@ -2475,7 +2475,6 @@ namespace JSC {
virtual bool isBindingNode() const { return false; }
virtual bool isAssignmentElementNode() const { return false; }
virtual bool isRestParameter() const { return false; }
virtual RegisterID* emitDirectBinding(BytecodeGenerator&, RegisterID*, ExpressionNode*) { return nullptr; }

virtual RegisterID* writableDirectBindingIfPossible(BytecodeGenerator&) const { return nullptr; }
virtual void finishDirectBindingAssignment(BytecodeGenerator&) const { }
Expand Down Expand Up @@ -2507,7 +2506,6 @@ namespace JSC {
};
void collectBoundIdentifiers(Vector<Identifier>&) const final;
void bindValue(BytecodeGenerator&, RegisterID*) const final;
RegisterID* emitDirectBinding(BytecodeGenerator&, RegisterID* dst, ExpressionNode*) final;
void toString(StringBuilder&) const final;

Vector<Entry> m_targetPatterns;
Expand Down

0 comments on commit 7681169

Please sign in to comment.