Skip to content
Permalink
Browse files
[JSC] Remove unnecessary mov bytecodes when performing simple object …
…pattern destructuring to variables

https://bugs.webkit.org/show_bug.cgi?id=220219

Reviewed by Alexey Shvayka.

JSTests:

* stress/object-pattern-simple-fast-path.js: Added.
(shouldBe):
(shouldThrow):
(test1):

Source/JavaScriptCore:

Currently, we are first puts object pattern's expression into temporary variable, and then, we store it into local variable register.

The following code

    ({ data } = object);

emits this kind of bytecode.

    get_by_id          dst:loc10, base:loc9, property:0
    mov                dst:loc6, src:loc10

However, this should be

    get_by_id          dst:loc6, base:loc9, property:0

We are emitting many unnecessary movs since this destructuring pattern is common. Increasing amount of mov (1) discourages inlining unnecessarily and (2) simply makes
bytecode memory large. Since this is very common pattern, we should carefully optimize it to remove such unnecessary movs.

This patch looks into pattern when performing object pattern destructuring. And avoid emitting mov when it is possible. There are some cases we cannot remove movs, so
this patch's writableDirectBindingIfPossible looks into whether this is possible (& profitable).

* bytecompiler/NodesCodegen.cpp:
(JSC::ObjectPatternNode::bindValue const):
(JSC::BindingNode::writableDirectBindingIfPossible const):
(JSC::BindingNode::finishDirectBindingAssignment const):
(JSC::AssignmentElementNode::writableDirectBindingIfPossible const):
(JSC::AssignmentElementNode::finishDirectBindingAssignment const):
* parser/Nodes.h:
(JSC::DestructuringPatternNode::writableDirectBindingIfPossible const):
(JSC::DestructuringPatternNode::finishDirectBindingAssignment const):

Canonical link: https://commits.webkit.org/232718@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@271121 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Constellation committed Jan 2, 2021
1 parent 9a400c0 commit 5629a596c77a98749acb993b28072d0ee1672720
Showing 5 changed files with 284 additions and 2 deletions.
@@ -1,3 +1,15 @@
2021-01-01 Yusuke Suzuki <ysuzuki@apple.com>

[JSC] Remove unnecessary mov bytecodes when performing simple object pattern destructuring to variables
https://bugs.webkit.org/show_bug.cgi?id=220219

Reviewed by Alexey Shvayka.

* stress/object-pattern-simple-fast-path.js: Added.
(shouldBe):
(shouldThrow):
(test1):

2021-01-02 Alexey Shvayka <shvaikalesh@gmail.com>

Improve error message for uninitialized |this| in derived constructor
@@ -0,0 +1,136 @@
function shouldBe(actual, expected) {
if (actual !== expected)
throw new Error('bad value: ' + actual);
}

function shouldThrow(func, errorMessage) {
var errorThrown = false;
var error = null;
try {
func();
} catch (e) {
errorThrown = true;
error = e;
}
if (!errorThrown)
throw new Error('not thrown');
if (String(error) !== errorMessage)
throw new Error(`bad error: ${String(error)}`);
}

function test1({ data }) {
return data;
}

shouldBe(test1({ data: 42 }), 42);

shouldThrow(() => {
const data = 42;
({ data } = { data: 50 });
}, `TypeError: Attempted to assign to readonly property.`);

(function () {
const { data } = { data: 50 };
shouldBe(data, 50);
}());

shouldThrow(() => {
const { data } = data;
}, `ReferenceError: Cannot access uninitialized variable.`);

shouldThrow(() => {
const { [throwing()]: data } = { data: 50 };
function throwing() {
data;
}
}, `ReferenceError: Cannot access uninitialized variable.`);

(function () {
let data = 42;
({ data } = { data: 50 });
shouldBe(data, 50);
}());

(function () {
let { data } = { data: 50 };
shouldBe(data, 50);
}());

shouldThrow(() => {
let { data } = data;
}, `ReferenceError: Cannot access uninitialized variable.`);

shouldThrow(() => {
let { [throwing()]: data } = { data: 50 };
function throwing() {
data;
}
}, `ReferenceError: Cannot access uninitialized variable.`);

shouldThrow(() => {
let { [data = 'data']: data } = { data: 50 };
}, `ReferenceError: Cannot access uninitialized variable.`);

(function () {
let { [43]: data } = { 43: 50 };
shouldBe(data, 50);
}());

(function () {
let called1 = false;
let called2 = false;
let data = 42;
({ [throwing()]: data, ...ok } = { data: 50 });
function throwing() {
called1 = true;
shouldBe(data, 42);
return {
toString() {
called2 = true;
shouldBe(data, 42);
return 'data';
}
};
}
shouldBe(data, 50);
shouldBe(called1, true);
shouldBe(called2, true);
}());

(function () {
let data = 'ok';
let obj;
({ [data]: data, ...obj } = { ok: 'bad', bad: 42 });
shouldBe(data, 'bad');
shouldBe(obj.bad, 42);
}());

(function () {
globalThis.testing = 42;
({ testing } = { testing: 50 });
shouldBe(testing, 50);
}());

(function () {
var { data } = { data: 50 };
shouldBe(data, 50);
}());

(function () {
var { data } = { data: data };
shouldBe(data, undefined);
}());

Reflect.defineProperty(globalThis, 'readOnly', {
value: 30
});

shouldThrow(() => {
'use strict';
({ readOnly } = { readOnly: 42 });
}, `TypeError: Attempted to assign to readonly property.`);

(function () {
({ readOnly } = { readOnly: 42 });
shouldBe(readOnly, 30);
}());
@@ -1,3 +1,41 @@
2021-01-01 Yusuke Suzuki <ysuzuki@apple.com>

[JSC] Remove unnecessary mov bytecodes when performing simple object pattern destructuring to variables
https://bugs.webkit.org/show_bug.cgi?id=220219

Reviewed by Alexey Shvayka.

Currently, we are first puts object pattern's expression into temporary variable, and then, we store it into local variable register.

The following code

({ data } = object);

emits this kind of bytecode.

get_by_id dst:loc10, base:loc9, property:0
mov dst:loc6, src:loc10

However, this should be

get_by_id dst:loc6, base:loc9, property:0

We are emitting many unnecessary movs since this destructuring pattern is common. Increasing amount of mov (1) discourages inlining unnecessarily and (2) simply makes
bytecode memory large. Since this is very common pattern, we should carefully optimize it to remove such unnecessary movs.

This patch looks into pattern when performing object pattern destructuring. And avoid emitting mov when it is possible. There are some cases we cannot remove movs, so
this patch's writableDirectBindingIfPossible looks into whether this is possible (& profitable).

* bytecompiler/NodesCodegen.cpp:
(JSC::ObjectPatternNode::bindValue const):
(JSC::BindingNode::writableDirectBindingIfPossible const):
(JSC::BindingNode::finishDirectBindingAssignment const):
(JSC::AssignmentElementNode::writableDirectBindingIfPossible const):
(JSC::AssignmentElementNode::finishDirectBindingAssignment const):
* parser/Nodes.h:
(JSC::DestructuringPatternNode::writableDirectBindingIfPossible const):
(JSC::DestructuringPatternNode::finishDirectBindingAssignment const):

2021-01-02 Alexey Shvayka <shvaikalesh@gmail.com>

Improve error message for uninitialized |this| in derived constructor
@@ -5264,7 +5264,39 @@ void ObjectPatternNode::bindValue(BytecodeGenerator& generator, RegisterID* rhs)
for (size_t i = 0; i < m_targetPatterns.size(); i++) {
const auto& target = m_targetPatterns[i];
if (target.bindingType == BindingType::Element) {
RefPtr<RegisterID> temp = generator.newTemporary();
// If the destructuring becomes get_by_id and mov, then we should store results directly to the local's binding.
// From
// get_by_id dst:loc10, base:loc9, property:0
// mov dst:loc6, src:loc10
// To
// get_by_id dst:loc6, base:loc9, property:0
auto writableDirectBindingIfPossible = [&]() -> RegisterID* {
// The following pattern is possible. In that case, after setting |data| local variable, we need to store property name into the set.
// So, old property name |data| result must be kept before setting it into |data|.
// ({ [data]: data, ...obj } = object);
if (m_containsRestElement && m_containsComputedProperty && target.propertyExpression)
return nullptr;
// default value can include a reference to local variable. So filling value to a local variable can differ result.
// We give up fast path if default value includes non constant.
// For example,
// ({ data = data } = object);
if (target.defaultValue && !target.defaultValue->isConstant())
return nullptr;
return target.pattern->writableDirectBindingIfPossible(generator);
};

auto finishDirectBindingAssignment = [&]() {
ASSERT(writableDirectBindingIfPossible());
target.pattern->finishDirectBindingAssignment(generator);
};

RefPtr<RegisterID> temp;
RegisterID* directBinding = writableDirectBindingIfPossible();
if (directBinding)
temp = directBinding;
else
temp = generator.newTemporary();

RefPtr<RegisterID> propertyName;
if (!target.propertyExpression) {
Optional<uint32_t> optionalIndex = parseIndex(target.propertyName);
@@ -5296,7 +5328,10 @@ void ObjectPatternNode::bindValue(BytecodeGenerator& generator, RegisterID* rhs)

if (target.defaultValue)
assignDefaultValueIfUndefined(generator, temp.get(), target.defaultValue);
target.pattern->bindValue(generator, temp.get());
if (directBinding)
finishDirectBindingAssignment();
else
target.pattern->bindValue(generator, temp.get());
} else {
ASSERT(target.bindingType == BindingType::RestElement);
ASSERT(i == m_targetPatterns.size() - 1);
@@ -5331,6 +5366,32 @@ void ObjectPatternNode::collectBoundIdentifiers(Vector<Identifier>& identifiers)
m_targetPatterns[i].pattern->collectBoundIdentifiers(identifiers);
}

RegisterID* BindingNode::writableDirectBindingIfPossible(BytecodeGenerator& generator) const
{
Variable var = generator.variable(m_boundProperty);
bool isReadOnly = var.isReadOnly() && m_bindingContext != AssignmentContext::ConstDeclarationStatement;
if (RegisterID* local = var.local()) {
if (m_bindingContext == AssignmentContext::AssignmentExpression) {
if (generator.needsTDZCheck(var))
return nullptr;
}
if (isReadOnly)
return nullptr;
return local;
}
return nullptr;
}

void BindingNode::finishDirectBindingAssignment(BytecodeGenerator& generator) const
{
ASSERT(writableDirectBindingIfPossible(generator));
Variable var = generator.variable(m_boundProperty);
RegisterID* local = var.local();
generator.emitProfileType(local, var, divotStart(), divotEnd());
if (m_bindingContext == AssignmentContext::DeclarationStatement || m_bindingContext == AssignmentContext::ConstDeclarationStatement)
generator.liftTDZCheckIfPossible(var);
}

void BindingNode::bindValue(BytecodeGenerator& generator, RegisterID* value) const
{
Variable var = generator.variable(m_boundProperty);
@@ -5375,6 +5436,32 @@ void BindingNode::collectBoundIdentifiers(Vector<Identifier>& identifiers) const
identifiers.append(m_boundProperty);
}

RegisterID* AssignmentElementNode::writableDirectBindingIfPossible(BytecodeGenerator& generator) const
{
if (!m_assignmentTarget->isResolveNode())
return nullptr;
ResolveNode* lhs = static_cast<ResolveNode*>(m_assignmentTarget);
Variable var = generator.variable(lhs->identifier());
bool isReadOnly = var.isReadOnly();
if (RegisterID* local = var.local()) {
if (generator.needsTDZCheck(var))
return nullptr;
if (isReadOnly)
return nullptr;
return local;
}
return nullptr;
}

void AssignmentElementNode::finishDirectBindingAssignment(BytecodeGenerator& generator) const
{
ASSERT_UNUSED(generator, writableDirectBindingIfPossible(generator));
ResolveNode* lhs = static_cast<ResolveNode*>(m_assignmentTarget);
Variable var = generator.variable(lhs->identifier());
RegisterID* local = var.local();
generator.emitProfileType(local, divotStart(), divotEnd());
}

void AssignmentElementNode::collectBoundIdentifiers(Vector<Identifier>&) const
{
}
@@ -2374,6 +2374,9 @@ namespace JSC {
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 { }

protected:
DestructuringPatternNode();
@@ -2460,6 +2463,9 @@ namespace JSC {

const JSTextPosition& divotStart() const { return m_divotStart; }
const JSTextPosition& divotEnd() const { return m_divotEnd; }

RegisterID* writableDirectBindingIfPossible(BytecodeGenerator&) const final;
void finishDirectBindingAssignment(BytecodeGenerator&) const;

private:
void collectBoundIdentifiers(Vector<Identifier>&) const final;
@@ -2499,6 +2505,9 @@ namespace JSC {
const JSTextPosition& divotStart() const { return m_divotStart; }
const JSTextPosition& divotEnd() const { return m_divotEnd; }

RegisterID* writableDirectBindingIfPossible(BytecodeGenerator&) const final;
void finishDirectBindingAssignment(BytecodeGenerator&) const;

private:
void collectBoundIdentifiers(Vector<Identifier>&) const final;
void bindValue(BytecodeGenerator&, RegisterID*) const final;

0 comments on commit 5629a59

Please sign in to comment.