Changeset 262354 in webkit

Timestamp:

May 30, 2020 8:20:40 PM (4 years ago)

Author:

ysuzuki@apple.com

Message:

[JSC] for-in should allocate new temporary register for base
​https://bugs.webkit.org/show_bug.cgi?id=212519
<rdar://problem/63722044>

Reviewed by Saam Barati.

JSTests:

• microbenchmarks/has-own-property-for-in-loop-with-heap-variable.js: Added.

(assert):
(test1.count):
(test1):

• microbenchmarks/has-own-property-for-in-loop-with-this.js: Added.

(assert):
(test1.count):
(test1):

• stress/for-in-body-replace-enumerable.js: Added.

(foo):

• stress/for-in-enumerable-shadow.js: Added.

(assert):
(test1.count):
(test1):

• stress/for-in-enumerable-this-arrow.js: Added.

(assert):
(test1):

Source/JavaScriptCore:

While r262233 keeps for-in's enumerated object in variable register if possible to use this register for heuristics driving an optimization,
for-in body can replace the content of this register during enumeration and confuse enumerator.

Instead, we record Variable information in StructureForInContext. This allows us to detect patterns using heap-variables too.
Further, this patch extends pattern-matching code to support ThisNode too.

• bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::pushStructureForInScope):

• bytecompiler/BytecodeGenerator.h:

(JSC::Variable::Variable):
(JSC::Variable::isResolved const):
(JSC::Variable::symbolTableConstantIndex const):
(JSC::Variable::ident const):
(JSC::Variable::offset const):
(JSC::Variable::isLocal const):
(JSC::Variable::local const):
(JSC::Variable::isReadOnly const):
(JSC::Variable::isSpecial const):
(JSC::Variable::isConst const):
(JSC::Variable::setIsReadOnly):
(JSC::Variable::operator== const):
(JSC::StructureForInContext::StructureForInContext):
(JSC::StructureForInContext::baseVariable const):
(JSC::StructureForInContext::base const): Deleted.

• bytecompiler/NodesCodegen.cpp:

(JSC::HasOwnPropertyFunctionCallDotNode::emitBytecode):
(JSC::ForInNode::emitBytecode):

• parser/ASTBuilder.h:

(JSC::ASTBuilder::makeFunctionCallNode):

• parser/Nodes.h:

(JSC::ExpressionNode::isThisNode const):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/microbenchmarks/has-own-property-for-in-loop-with-heap-variable.js (added)
• JSTests/microbenchmarks/has-own-property-for-in-loop-with-this.js (added)
• JSTests/stress/for-in-body-replace-enumerable.js (added)
• JSTests/stress/for-in-enumerable-shadow.js (added)
• JSTests/stress/for-in-enumerable-this-arrow.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp (modified) (1 diff)
• Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h (modified) (6 diffs)
• Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp (modified) (3 diffs)
• Source/JavaScriptCore/parser/ASTBuilder.h (modified) (2 diffs)
• Source/JavaScriptCore/parser/Nodes.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-05-30  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] for-in should allocate new temporary register for base
+        https://bugs.webkit.org/show_bug.cgi?id=212519
+        <rdar://problem/63722044>
+
+        Reviewed by Saam Barati.
+
+        * microbenchmarks/has-own-property-for-in-loop-with-heap-variable.js: Added.
+        (assert):
+        (test1.count):
+        (test1):
+        * microbenchmarks/has-own-property-for-in-loop-with-this.js: Added.
+        (assert):
+        (test1.count):
+        (test1):
+        * stress/for-in-body-replace-enumerable.js: Added.
+        (foo):
+        * stress/for-in-enumerable-shadow.js: Added.
+        (assert):
+        (test1.count):
+        (test1):
+        * stress/for-in-enumerable-this-arrow.js: Added.
+        (assert):
+        (test1):
+
 2020-05-29  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-05-30  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] for-in should allocate new temporary register for base
+        https://bugs.webkit.org/show_bug.cgi?id=212519
+        <rdar://problem/63722044>
+
+        Reviewed by Saam Barati.
+
+        While r262233 keeps for-in's enumerated object in variable register if possible to use this register for heuristics driving an optimization,
+        for-in body can replace the content of this register during enumeration and confuse enumerator.
+
+        Instead, we record Variable information in StructureForInContext. This allows us to detect patterns using heap-variables too.
+        Further, this patch extends pattern-matching code to support ThisNode too.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::pushStructureForInScope):
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::Variable::Variable):
+        (JSC::Variable::isResolved const):
+        (JSC::Variable::symbolTableConstantIndex const):
+        (JSC::Variable::ident const):
+        (JSC::Variable::offset const):
+        (JSC::Variable::isLocal const):
+        (JSC::Variable::local const):
+        (JSC::Variable::isReadOnly const):
+        (JSC::Variable::isSpecial const):
+        (JSC::Variable::isConst const):
+        (JSC::Variable::setIsReadOnly):
+        (JSC::Variable::operator== const):
+        (JSC::StructureForInContext::StructureForInContext):
+        (JSC::StructureForInContext::baseVariable const):
+        (JSC::StructureForInContext::base const): Deleted.
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::HasOwnPropertyFunctionCallDotNode::emitBytecode):
+        (JSC::ForInNode::emitBytecode):
+        * parser/ASTBuilder.h:
+        (JSC::ASTBuilder::makeFunctionCallNode):
+        * parser/Nodes.h:
+        (JSC::ExpressionNode::isThisNode const):
+
 2020-05-30  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -4534 +4534 @@
 }
 
-void BytecodeGenerator::pushStructureForInScope(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister, RegisterID* baseRegister)
+void BytecodeGenerator::pushStructureForInScope(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister, Optional<Variable> baseVariable)
 {
     if (!localRegister)
         return;
     unsigned bodyBytecodeStartOffset = instructions().size();
-    m_forInContextStack.append(adoptRef(*new StructureForInContext(localRegister, indexRegister, propertyRegister, enumeratorRegister, baseRegister, bodyBytecodeStartOffset)));
+    m_forInContextStack.append(adoptRef(*new StructureForInContext(localRegister, indexRegister, propertyRegister, enumeratorRegister, baseVariable, bodyBytecodeStartOffset)));
 }
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h

@@ -93 +93 @@
     };
 
+    class Variable {
+    public:
+        enum VariableKind { NormalVariable, SpecialVariable };
+
+        Variable() = default;
+        
+        Variable(const Identifier& ident)
+            : m_ident(ident)
+            , m_local(nullptr)
+            , m_attributes(0)
+            , m_kind(NormalVariable) // This is somewhat meaningless here for this kind of Variable.
+            , m_symbolTableConstantIndex(0) // This is meaningless here for this kind of Variable.
+            , m_isLexicallyScoped(false)
+        {
+        }
+
+        Variable(const Identifier& ident, VarOffset offset, RegisterID* local, unsigned attributes, VariableKind kind, int symbolTableConstantIndex, bool isLexicallyScoped)
+            : m_ident(ident)
+            , m_offset(offset)
+            , m_local(local)
+            , m_attributes(attributes)
+            , m_kind(kind)
+            , m_symbolTableConstantIndex(symbolTableConstantIndex)
+            , m_isLexicallyScoped(isLexicallyScoped)
+        {
+        }
+
+        // If it's unset, then it is a non-locally-scoped variable. If it is set, then it could be
+        // a stack variable, a scoped variable in a local scope, or a variable captured in the
+        // direct arguments object.
+        bool isResolved() const { return !!m_offset; }
+        int symbolTableConstantIndex() const { ASSERT(isResolved() && !isSpecial()); return m_symbolTableConstantIndex; }
+        
+        const Identifier& ident() const { return m_ident; }
+        
+        VarOffset offset() const { return m_offset; }
+        bool isLocal() const { return m_offset.isStack(); }
+        RegisterID* local() const { return m_local; }
+
+        bool isReadOnly() const { return m_attributes & PropertyAttribute::ReadOnly; }
+        bool isSpecial() const { return m_kind != NormalVariable; }
+        bool isConst() const { return isReadOnly() && m_isLexicallyScoped; }
+        void setIsReadOnly() { m_attributes |= PropertyAttribute::ReadOnly; }
+
+        void dump(PrintStream&) const;
+
+        bool operator==(const Variable& other) const
+        {
+            return m_ident == other.m_ident
+                && m_offset == other.m_offset
+                && m_local == other.m_local
+                && m_attributes == other.m_attributes
+                && m_kind == other.m_kind
+                && m_symbolTableConstantIndex == other.m_symbolTableConstantIndex
+                && m_isLexicallyScoped == other.m_isLexicallyScoped;
+        }
+
+    private:
+        Identifier m_ident;
+        VarOffset m_offset { };
+        RegisterID* m_local { nullptr };
+        unsigned m_attributes { 0 };
+        VariableKind m_kind { NormalVariable };
+        int m_symbolTableConstantIndex { 0 }; // This is meaningless here for this default NormalVariable kind of Variable.
+        bool m_isLexicallyScoped { false };
+    };
+
     // https://tc39.github.io/ecma262/#sec-completion-record-specification-type
     //
@@ -242 +309 @@
         using HasOwnPropertyJumpInst = std::tuple<unsigned, unsigned>;
 
-        StructureForInContext(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister, RegisterID* base, unsigned bodyBytecodeStartOffset)
+        StructureForInContext(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister, Optional<Variable> baseVariable, unsigned bodyBytecodeStartOffset)
             : ForInContext(localRegister, Type::StructureForIn, bodyBytecodeStartOffset)
             , m_indexRegister(indexRegister)
             , m_propertyRegister(propertyRegister)
             , m_enumeratorRegister(enumeratorRegister)
-            , m_base(base)
+            , m_baseVariable(baseVariable)
         {
         }
@@ -254 +321 @@
         RegisterID* property() const { return m_propertyRegister.get(); }
         RegisterID* enumerator() const { return m_enumeratorRegister.get(); }
-        RegisterID* base() const { return m_base.get(); }
+        const Optional<Variable>& baseVariable() const { return m_baseVariable; }
 
         void addGetInst(unsigned instIndex, int propertyRegIndex)
@@ -277 +344 @@
         RefPtr<RegisterID> m_propertyRegister;
         RefPtr<RegisterID> m_enumeratorRegister;
-        RefPtr<RegisterID> m_base;
+        Optional<Variable> m_baseVariable;
         Vector<GetInst> m_getInsts;
         Vector<InInst> m_inInsts;
@@ -310 +377 @@
         Ref<Label> start;
         TryData* tryData;
-    };
-
-    class Variable {
-    public:
-        enum VariableKind { NormalVariable, SpecialVariable };
-
-        Variable()
-            : m_offset()
-            , m_local(nullptr)
-            , m_attributes(0)
-            , m_kind(NormalVariable)
-            , m_symbolTableConstantIndex(0) // This is meaningless here for this kind of Variable.
-            , m_isLexicallyScoped(false)
-        {
-        }
-        
-        Variable(const Identifier& ident)
-            : m_ident(ident)
-            , m_local(nullptr)
-            , m_attributes(0)
-            , m_kind(NormalVariable) // This is somewhat meaningless here for this kind of Variable.
-            , m_symbolTableConstantIndex(0) // This is meaningless here for this kind of Variable.
-            , m_isLexicallyScoped(false)
-        {
-        }
-
-        Variable(const Identifier& ident, VarOffset offset, RegisterID* local, unsigned attributes, VariableKind kind, int symbolTableConstantIndex, bool isLexicallyScoped)
-            : m_ident(ident)
-            , m_offset(offset)
-            , m_local(local)
-            , m_attributes(attributes)
-            , m_kind(kind)
-            , m_symbolTableConstantIndex(symbolTableConstantIndex)
-            , m_isLexicallyScoped(isLexicallyScoped)
-        {
-        }
-
-        // If it's unset, then it is a non-locally-scoped variable. If it is set, then it could be
-        // a stack variable, a scoped variable in a local scope, or a variable captured in the
-        // direct arguments object.
-        bool isResolved() const { return !!m_offset; }
-        int symbolTableConstantIndex() const { ASSERT(isResolved() && !isSpecial()); return m_symbolTableConstantIndex; }
-        
-        const Identifier& ident() const { return m_ident; }
-        
-        VarOffset offset() const { return m_offset; }
-        bool isLocal() const { return m_offset.isStack(); }
-        RegisterID* local() const { return m_local; }
-
-        bool isReadOnly() const { return m_attributes & PropertyAttribute::ReadOnly; }
-        bool isSpecial() const { return m_kind != NormalVariable; }
-        bool isConst() const { return isReadOnly() && m_isLexicallyScoped; }
-        void setIsReadOnly() { m_attributes |= PropertyAttribute::ReadOnly; }
-
-        void dump(PrintStream&) const;
-
-    private:
-        Identifier m_ident;
-        VarOffset m_offset;
-        RegisterID* m_local;
-        unsigned m_attributes;
-        VariableKind m_kind;
-        int m_symbolTableConstantIndex;
-        bool m_isLexicallyScoped;
     };
 
@@ -1026 +1029 @@
         void pushIndexedForInScope(RegisterID* local, RegisterID* index);
         void popIndexedForInScope(RegisterID* local);
-        void pushStructureForInScope(RegisterID* local, RegisterID* index, RegisterID* property, RegisterID* enumerator, RegisterID* base);
+        void pushStructureForInScope(RegisterID* local, RegisterID* index, RegisterID* property, RegisterID* enumerator, Optional<Variable> base);
         void popStructureForInScope(RegisterID* local);
 

trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

@@ -1853 +1853 @@
     }
 
-    if (structureContext && structureContext->base() == base.get()) {
+    auto canUseFastHasOwnProperty = [&] {
+        if (!structureContext)
+            return false;
+        if (!structureContext->baseVariable())
+            return false;
+        if (m_base->isResolveNode())
+            return generator.variable(static_cast<ResolveNode*>(m_base)->identifier()) == structureContext->baseVariable().value();
+        if (m_base->isThisNode()) {
+            // After generator.ensureThis (which must be invoked in |base|'s materialization), we can ensure that |this| is in local this-register.
+            ASSERT(base);
+            return generator.variable(generator.propertyNames().thisIdentifier, ThisResolutionType::Local) == structureContext->baseVariable().value();
+        }
+        return false;
+    };
+
+    if (canUseFastHasOwnProperty()) {
+        // It is possible that base register is variable and each for-in body replaces JS object in the base register with a different one.
+        // Even though, this is OK since HasOwnStructureProperty will reject the replaced JS object.
         Ref<Label> realCall = generator.newLabel();
         Ref<Label> end = generator.newLabel();
@@ -3748 +3765 @@
         generator.emitNode(generator.ignoredResult(), m_lexpr);
 
+    RefPtr<RegisterID> base = generator.newTemporary();
     RefPtr<RegisterID> length;
     RefPtr<RegisterID> enumerator;
 
-    RefPtr<RegisterID> base = generator.emitNode(m_expr);
+    generator.emitNode(base.get(), m_expr);
     RefPtr<RegisterID> local = this->tryGetBoundLocal(generator);
     RefPtr<RegisterID> enumeratorIndex;
+
+    Optional<Variable> baseVariable;
+    if (m_expr->isResolveNode())
+        baseVariable = generator.variable(static_cast<ResolveNode*>(m_expr)->identifier());
+    else if (m_expr->isThisNode()) {
+        // After generator.ensureThis (which must be invoked in |base|'s materialization), we can ensure that |this| is in local this-register.
+        ASSERT(base);
+        baseVariable = generator.variable(generator.propertyNames().thisIdentifier, ThisResolutionType::Local);
+    }
 
     // Pause at the assignment expression for each for..in iteration.
@@ -3829 +3856 @@
         generator.emitProfileControlFlow(profilerStartOffset);
 
-        generator.pushStructureForInScope(local.get(), enumeratorIndex.get(), propertyName.get(), enumerator.get(), base.get());
+        generator.pushStructureForInScope(local.get(), enumeratorIndex.get(), propertyName.get(), enumerator.get(), baseVariable);
         generator.emitNode(dst, m_statement);
         generator.popStructureForInScope(local.get());

trunk/Source/JavaScriptCore/parser/ASTBuilder.h

@@ -1443 +1443 @@
         // FIXME: This check is only needed because we haven't taught the bytecode generator to inline
         // Reflect.apply yet. See https://bugs.webkit.org/show_bug.cgi?id=190668.
-        if (!dot->base()->isResolveNode() || static_cast<ResolveNode*>(dot->base())->identifier() != "Reflect")
+        if (!dot->base()->isResolveNode() || static_cast<ResolveNode*>(dot->base())->identifier() != m_vm.propertyNames->Reflect)
             node = new (m_parserArena) ApplyFunctionCallDotNode(location, dot->base(), dot->identifier(), args, divot, divotStart, divotEnd, callOrApplyChildDepth);
     } else if (!previousBaseWasSuper 
@@ -1451 +1451 @@
         && args->m_listNode->m_expr->isResolveNode()
         && !args->m_listNode->m_next
-        && dot->base()->isResolveNode()
-        && static_cast<ResolveNode*>(dot->base())->identifier() != "Reflect") {
+        && ((dot->base()->isResolveNode() && static_cast<ResolveNode*>(dot->base())->identifier() != m_vm.propertyNames->Reflect) || dot->base()->isThisNode())) {
         // We match the AST pattern:
-        // <resolveNode>.hasOwnProperty(<resolveNode>)
+        // <resolveNode|thisNode>.hasOwnProperty(<resolveNode>)
         // i.e:
         // o.hasOwnProperty(p)

trunk/Source/JavaScriptCore/parser/Nodes.h

@@ -201 +201 @@
         virtual bool isSubtract() const { return false; }
         virtual bool isBoolean() const { return false; }
+        virtual bool isThisNode() const { return false; }
         virtual bool isSpreadExpression() const { return false; }
         virtual bool isSuperNode() const { return false; }
@@ -613 +614 @@
 
     private:
+        bool isThisNode() const final { return true; }
         RegisterID* emitBytecode(BytecodeGenerator&, RegisterID* = nullptr) final;
     };