Changeset 172741 in webkit

Timestamp:

Aug 18, 2014, 7:48:00 PM (11 years ago)

Author:

fpizlo@apple.com

Message:

REGRESSION(r172401): for-in optimization no longer works at all
​https://bugs.webkit.org/show_bug.cgi?id=136056

Reviewed by Mark Hahnenberg.

This is a partial roll-out of r172401. It turns out that the fix wasn't actually fixing a
real bug (since it's fine to use op_get_direct_pname on the wrong base because it has a
structure check) and it was actually breaking the entire for-in optimization (since there is
no way that we can statically prove that the base matches, because the base we see is a
newly created temporary, and anyway doing it right would be really hard in our bytecode
because it's 3AC form).

But, I added a new test for the problem, and kept the original test. Both the old test and
the new test prove that r172401 wasn't fixing what it thought it was fixing. To the extent
that it resolved crashes it was because it just disabled the for-in optimization entirely.

• bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::emitGetByVal):
(JSC::BytecodeGenerator::pushIndexedForInScope):
(JSC::BytecodeGenerator::pushStructureForInScope):

• bytecompiler/BytecodeGenerator.h:

(JSC::ForInContext::ForInContext):
(JSC::StructureForInContext::StructureForInContext):
(JSC::IndexedForInContext::IndexedForInContext):
(JSC::ForInContext::base): Deleted.

• bytecompiler/NodesCodegen.cpp:

(JSC::ForInNode::emitMultiLoopBytecode):

• tests/stress/for-in-base-reassigned.js: Added.
• tests/stress/for-in-base-reassigned-later.js: Added.
• tests/stress/for-in-base-reassigned-later-and-change-structure.js: Added.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecompiler/BytecodeGenerator.cpp (modified) (3 diffs)
• bytecompiler/BytecodeGenerator.h (modified) (5 diffs)
• bytecompiler/NodesCodegen.cpp (modified) (2 diffs)
• tests/stress/for-in-base-reassigned-later-and-change-structure.js (added)
• tests/stress/for-in-base-reassigned-later.js (added)
• tests/stress/for-in-base-reassigned.js (added)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2014-08-18  Filip Pizlo  <fpizlo@apple.com>
+
+        REGRESSION(r172401): for-in optimization no longer works at all
+        https://bugs.webkit.org/show_bug.cgi?id=136056
+
+        Reviewed by Mark Hahnenberg.
+        
+        This is a partial roll-out of r172401. It turns out that the fix wasn't actually fixing a
+        real bug (since it's fine to use op_get_direct_pname on the wrong base because it has a
+        structure check) and it was actually breaking the entire for-in optimization (since there is
+        no way that we can statically prove that the base matches, because the base we see is a
+        newly created temporary, and anyway doing it right would be really hard in our bytecode
+        because it's 3AC form).
+        
+        But, I added a new test for the problem, and kept the original test. Both the old test and
+        the new test prove that r172401 wasn't fixing what it thought it was fixing. To the extent
+        that it resolved crashes it was because it just disabled the for-in optimization entirely.
+
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::emitGetByVal):
+        (JSC::BytecodeGenerator::pushIndexedForInScope):
+        (JSC::BytecodeGenerator::pushStructureForInScope):
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::ForInContext::ForInContext):
+        (JSC::StructureForInContext::StructureForInContext):
+        (JSC::IndexedForInContext::IndexedForInContext):
+        (JSC::ForInContext::base): Deleted.
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::ForInNode::emitMultiLoopBytecode):
+        * tests/stress/for-in-base-reassigned.js: Added.
+        * tests/stress/for-in-base-reassigned-later.js: Added.
+        * tests/stress/for-in-base-reassigned-later-and-change-structure.js: Added.
+
 2014-08-18  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -1423 +1423 @@
     for (size_t i = m_forInContextStack.size(); i > 0; i--) {
         ForInContext* context = m_forInContextStack[i - 1].get();
-        if (context->base() != base)
-            continue;
-
         if (context->local() != property)
             continue;
@@ -2587 +2584 @@
 }
 
-void BytecodeGenerator::pushIndexedForInScope(RegisterID* baseRegister, RegisterID* localRegister, RegisterID* indexRegister)
+void BytecodeGenerator::pushIndexedForInScope(RegisterID* localRegister, RegisterID* indexRegister)
 {
     if (!localRegister)
         return;
-    m_forInContextStack.append(std::make_unique<IndexedForInContext>(baseRegister, localRegister, indexRegister));
+    m_forInContextStack.append(std::make_unique<IndexedForInContext>(localRegister, indexRegister));
 }
 
@@ -2601 +2598 @@
 }
 
-void BytecodeGenerator::pushStructureForInScope(RegisterID* baseRegister, RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister)
+void BytecodeGenerator::pushStructureForInScope(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister)
 {
     if (!localRegister)
         return;
-    m_forInContextStack.append(std::make_unique<StructureForInContext>(baseRegister, localRegister, indexRegister, propertyRegister, enumeratorRegister));
+    m_forInContextStack.append(std::make_unique<StructureForInContext>(localRegister, indexRegister, propertyRegister, enumeratorRegister));
 }
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h

@@ -100 +100 @@
     class ForInContext {
     public:
-        ForInContext(RegisterID* baseRegister, RegisterID* localRegister)
-            : m_baseRegister(baseRegister)
-            , m_localRegister(localRegister)
+        ForInContext(RegisterID* localRegister)
+            : m_localRegister(localRegister)
             , m_isValid(true)
         {
@@ -120 +119 @@
         virtual ForInContextType type() const = 0;
 
-        RegisterID* base() const { return m_baseRegister.get(); }
         RegisterID* local() const { return m_localRegister.get(); }
 
     private:
-        RefPtr<RegisterID> m_baseRegister;
         RefPtr<RegisterID> m_localRegister;
         bool m_isValid;
@@ -131 +128 @@
     class StructureForInContext : public ForInContext {
     public:
-        StructureForInContext(RegisterID* baseRegister, RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister)
-            : ForInContext(baseRegister, localRegister)
+        StructureForInContext(RegisterID* localRegister, RegisterID* indexRegister, RegisterID* propertyRegister, RegisterID* enumeratorRegister)
+            : ForInContext(localRegister)
             , m_indexRegister(indexRegister)
             , m_propertyRegister(propertyRegister)
@@ -156 +153 @@
     class IndexedForInContext : public ForInContext {
     public:
-        IndexedForInContext(RegisterID* baseRegister, RegisterID* localRegister, RegisterID* indexRegister)
-            : ForInContext(baseRegister, localRegister)
+        IndexedForInContext(RegisterID* localRegister, RegisterID* indexRegister)
+            : ForInContext(localRegister)
             , m_indexRegister(indexRegister)
         {
@@ -528 +525 @@
         void popFinallyContext();
 
-        void pushIndexedForInScope(RegisterID* base, RegisterID* local, RegisterID* index);
+        void pushIndexedForInScope(RegisterID* local, RegisterID* index);
         void popIndexedForInScope(RegisterID* local);
-        void pushStructureForInScope(RegisterID* base, RegisterID* local, RegisterID* index, RegisterID* property, RegisterID* enumerator);
+        void pushStructureForInScope(RegisterID* local, RegisterID* index, RegisterID* property, RegisterID* enumerator);
         void popStructureForInScope(RegisterID* local);
         void invalidateForInContextForLocal(RegisterID* local);

trunk/Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

@@ -2071 +2071 @@
         this->emitLoopHeader(generator, propertyName.get());
 
-        generator.pushIndexedForInScope(base.get(), local.get(), i.get());
+        generator.pushIndexedForInScope(local.get(), i.get());
         generator.emitNode(dst, m_statement);
         generator.popIndexedForInScope(local.get());
@@ -2105 +2105 @@
         this->emitLoopHeader(generator, propertyName.get());
 
-        generator.pushStructureForInScope(base.get(), local.get(), i.get(), propertyName.get(), structureEnumerator.get());
+        generator.pushStructureForInScope(local.get(), i.get(), propertyName.get(), structureEnumerator.get());
         generator.emitNode(dst, m_statement);
         generator.popStructureForInScope(local.get());