Changeset 264750 in webkit

Timestamp:

Jul 23, 2020, 1:25:12 AM (6 years ago)

Author:

Alexey Shvayka

Message:

Remove ArrayNode::m_optional
​https://bugs.webkit.org/show_bug.cgi?id=214294

Reviewed by Darin Adler.

JSTests:

• microbenchmarks/destructuring-array-literal.js: Added.
• microbenchmarks/function-dot-apply-array-literal.js: Added.
• stress/apply-second-argument-must-be-array-like.js:
• stress/destructuring-assignment-syntax.js:

Source/JavaScriptCore:

m_optional, which dates back to KJS era, means "is this an array with optional trailing comma,
with elision, or an empty array". It was used by ArrayNode::streamTo() to preserve a trailing
comma when converting array node to source string, as well as in few other places,
before ECMA-262 clarified trailing comma in array literals [1].

Currently, m_optional is used only by ArrayNode::isSimpleArray(), along with m_elision.
Checking m_elision is enough since trailing comma doesn't add extra undefined element.

This patch completely removes m_optional, speeding up destructuring and function.apply()
with empty arrays and arrays with trailing commas by ~55% and a factor of 11 respectively.
Reflect.apply() optimization (​https://webkit.org/b/190668) will also benefit from this change.

Also, this change converts isSpreadExpression() check to an ASSERT (was enabled by r196323).

[1]: ​https://tc39.es/ecma262/#sec-array-initializer

• bytecompiler/NodesCodegen.cpp:

(JSC::ArrayNode::isSimpleArray const):
(JSC::ArrayNode::toArgumentList const):
(JSC::ArrayNode::emitDirectBinding):

• parser/NodeConstructors.h:

(JSC::ArrayNode::ArrayNode):

• parser/Nodes.h:

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/microbenchmarks/destructuring-array-literal.js (added)
• JSTests/microbenchmarks/function-dot-apply-array-literal.js (added)
• JSTests/stress/apply-second-argument-must-be-array-like.js (modified) (2 diffs)
• JSTests/stress/destructuring-assignment-syntax.js (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp (modified) (3 diffs)
• Source/JavaScriptCore/parser/NodeConstructors.h (modified) (3 diffs)
• Source/JavaScriptCore/parser/Nodes.h (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-07-23  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        Remove ArrayNode::m_optional
+        https://bugs.webkit.org/show_bug.cgi?id=214294
+
+        Reviewed by Darin Adler.
+
+        * microbenchmarks/destructuring-array-literal.js: Added.
+        * microbenchmarks/function-dot-apply-array-literal.js: Added.
+        * stress/apply-second-argument-must-be-array-like.js:
+        * stress/destructuring-assignment-syntax.js:
+
 2020-07-22  Angelos Oikonomopoulos  <angelos@igalia.com>
 

trunk/JSTests/stress/apply-second-argument-must-be-array-like.js

@@ -19 +19 @@
 }
 
-function shouldNotThrow(expr) {
-    let testFunc = new Function(expr);
-    for (let i = 0; i < 10000; i++) {
-        let error;
-        try {
-            testFunc();
-        } catch (e) {
-            error = e;
-        }
-        assert(!error);
-    }
-}
-
 function foo() { }
 
@@ -43 +30 @@
 shouldThrow("foo.apply(undefined, Symbol())");
 
-shouldNotThrow("foo.apply(undefined, undefined)");
-shouldNotThrow("foo.apply(undefined, null)");
-shouldNotThrow("foo.apply(undefined, {})");
-shouldNotThrow("foo.apply(undefined, [])");
-shouldNotThrow("foo.apply(undefined, function(){})");
+function bar() {
+    return arguments.length;
+}
+
+for (let i = 0; i < 10000; i++) {
+    new Function(`
+        assert(bar.apply(undefined, undefined) === 0);
+        assert(bar.apply(undefined, null) === 0);
+        assert(bar.apply(undefined, {}) === 0);
+        assert(bar.apply(undefined, []) === 0);
+        assert(bar.apply(undefined, function() {}) === 0);
+    `)();
+}

trunk/JSTests/stress/destructuring-assignment-syntax.js

@@ -22 +22 @@
         throw new Error("Bad error: " + String(error));
 }
+
+testSyntax("[] = []");
+testSyntax("[] = [,]");
+testSyntax("[,] = [,]");
+testSyntax("[,] = []");
 
 testSyntax("({ a: this.a } = {})");

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-07-23  Alexey Shvayka  <shvaikalesh@gmail.com>
+
+        Remove ArrayNode::m_optional
+        https://bugs.webkit.org/show_bug.cgi?id=214294
+
+        Reviewed by Darin Adler.
+
+        m_optional, which dates back to KJS era, means "is this an array with optional trailing comma,
+        with elision, or an empty array". It was used by ArrayNode::streamTo() to preserve a trailing
+        comma when converting array node to source string, as well as in few other places,
+        before ECMA-262 clarified trailing comma in array literals [1].
+
+        Currently, m_optional is used only by ArrayNode::isSimpleArray(), along with m_elision.
+        Checking m_elision is enough since trailing comma doesn't add extra `undefined` element.
+
+        This patch completely removes m_optional, speeding up destructuring and function.apply()
+        with empty arrays and arrays with trailing commas by ~55% and a factor of 11 respectively.
+        Reflect.apply() optimization (https://webkit.org/b/190668) will also benefit from this change.
+
+        Also, this change converts isSpreadExpression() check to an ASSERT (was enabled by r196323).
+
+        [1]: https://tc39.es/ecma262/#sec-array-initializer
+
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::ArrayNode::isSimpleArray const):
+        (JSC::ArrayNode::toArgumentList const):
+        (JSC::ArrayNode::emitDirectBinding):
+        * parser/NodeConstructors.h:
+        (JSC::ArrayNode::ArrayNode):
+        * parser/Nodes.h:
+
 2020-07-23  Alexey Shvayka  <shvaikalesh@gmail.com>
 

trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

@@ -507 +507 @@
 bool ArrayNode::isSimpleArray() const
 {
-    if (m_elision || m_optional)
+    if (m_elision)
         return false;
     for (ElementNode* ptr = m_element; ptr; ptr = ptr->next()) {
@@ -520 +520 @@
 ArgumentListNode* ArrayNode::toArgumentList(ParserArena& parserArena, int lineNumber, int startPosition) const
 {
-    ASSERT(!m_elision && !m_optional);
+    ASSERT(!m_elision);
     ElementNode* ptr = m_element;
     if (!ptr)
@@ -5104 +5104 @@
     for (; elementNodes; elementNodes = elementNodes->next()) {
         ExpressionNode* value = elementNodes->value();
-        if (value->isSpreadExpression())
-            return nullptr;
+        ASSERT(!value->isSpreadExpression());
         elements.append(value);
     }

trunk/Source/JavaScriptCore/parser/NodeConstructors.h

@@ -227 +227 @@
         , m_element(nullptr)
         , m_elision(elision)
-        , m_optional(true)
     {
     }
@@ -235 +234 @@
         , m_element(element)
         , m_elision(0)
-        , m_optional(false)
     {
     }
@@ -243 +241 @@
         , m_element(element)
         , m_elision(elision)
-        , m_optional(true)
     {
     }

trunk/Source/JavaScriptCore/parser/Nodes.h

@@ -717 +717 @@
         ElementNode* m_element;
         int m_elision;
-        bool m_optional;
     };