- Revision
- 271121
- Author
- [email protected]
- Date
- 2021-01-02 12:46:24 -0800 (Sat, 02 Jan 2021)
Log Message
[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/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (271120 => 271121)
--- trunk/JSTests/ChangeLog 2021-01-02 19:27:42 UTC (rev 271120)
+++ trunk/JSTests/ChangeLog 2021-01-02 20:46:24 UTC (rev 271121)
@@ -1,3 +1,15 @@
+2021-01-01 Yusuke Suzuki <[email protected]>
+
+ [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 <[email protected]>
Improve error message for uninitialized |this| in derived constructor
Added: trunk/JSTests/stress/object-pattern-simple-fast-path.js (0 => 271121)
--- trunk/JSTests/stress/object-pattern-simple-fast-path.js (rev 0)
+++ trunk/JSTests/stress/object-pattern-simple-fast-path.js 2021-01-02 20:46:24 UTC (rev 271121)
@@ -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 = ""
+ ({ 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 = ""
+ ({ 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: 50 };
+}, `ReferenceError: Cannot access uninitialized variable.`);
+
+(function () {
+ let { [43]: data } = { 43: 50 };
+ shouldBe(data, 50);
+}());
+
+(function () {
+ let called1 = false;
+ let called2 = false;
+ let data = ""
+ ({ [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 = '';
+ 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);
+}());
Modified: trunk/Source/_javascript_Core/ChangeLog (271120 => 271121)
--- trunk/Source/_javascript_Core/ChangeLog 2021-01-02 19:27:42 UTC (rev 271120)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-01-02 20:46:24 UTC (rev 271121)
@@ -1,3 +1,41 @@
+2021-01-01 Yusuke Suzuki <[email protected]>
+
+ [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 <[email protected]>
Improve error message for uninitialized |this| in derived constructor
Modified: trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp (271120 => 271121)
--- trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp 2021-01-02 19:27:42 UTC (rev 271120)
+++ trunk/Source/_javascript_Core/bytecompiler/NodesCodegen.cpp 2021-01-02 20:46:24 UTC (rev 271121)
@@ -5264,7 +5264,39 @@
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 = "" } = 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 @@
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 @@
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 @@
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
{
}
Modified: trunk/Source/_javascript_Core/parser/Nodes.h (271120 => 271121)
--- trunk/Source/_javascript_Core/parser/Nodes.h 2021-01-02 19:27:42 UTC (rev 271120)
+++ trunk/Source/_javascript_Core/parser/Nodes.h 2021-01-02 20:46:24 UTC (rev 271121)
@@ -2374,6 +2374,9 @@
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 @@
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 @@
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;
