Changeset 179746 in webkit

Timestamp:

Feb 6, 2015, 8:04:39 AM (12 years ago)

Author:

fpizlo@apple.com

Message:

Remove BytecodeGenerator::preserveLastVar() and replace it with a more robust mechanism for preserving non-temporary registers
​https://bugs.webkit.org/show_bug.cgi?id=141211

Reviewed by Mark Lam.

Previously, the way non-temporary registers were preserved (i.e. not reclaimed anytime
we did newTemporary()) by calling preserveLastVar() after all non-temps are created. It
would raise the refcount on the last (highest-numbered) variable created, and rely on
the fact that register reclamation started at higher-numbered registers and worked its
way down. So any retained register would block any lower-numbered registers from being
reclaimed.

Also, preserveLastVar() sets a thing called m_firstConstantIndex. It's unused.

This removes preserveLastVar() and makes addVar() retain each register it creates. This
is more explicit, since addVar() is the mechanism for creating non-temporary registers.

To make this work I had to remove an assertion that Register::setIndex() can only be
called when the refcount is zero. This method might be called after a var is created to
change its index. This previously worked because preserveLastVar() would be called after
we had already made all index changes, so the vars would still have refcount zero. Now
they have refcount 1. I think it's OK to lose this assertion; I can't remember this
assertion ever firing in a way that alerted me to a serious issue.

• bytecompiler/BytecodeGenerator.cpp:

(JSC::BytecodeGenerator::BytecodeGenerator):
(JSC::BytecodeGenerator::preserveLastVar): Deleted.

• bytecompiler/BytecodeGenerator.h:

(JSC::BytecodeGenerator::addVar):

• bytecompiler/RegisterID.h:

(JSC::RegisterID::setIndex):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecompiler/BytecodeGenerator.cpp (modified) (3 diffs)
• bytecompiler/BytecodeGenerator.h (modified) (3 diffs)
• bytecompiler/RegisterID.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-02-04  Filip Pizlo  <fpizlo@apple.com>
+
+        Remove BytecodeGenerator::preserveLastVar() and replace it with a more robust mechanism for preserving non-temporary registers
+        https://bugs.webkit.org/show_bug.cgi?id=141211
+
+        Reviewed by Mark Lam.
+
+        Previously, the way non-temporary registers were preserved (i.e. not reclaimed anytime
+        we did newTemporary()) by calling preserveLastVar() after all non-temps are created. It
+        would raise the refcount on the last (highest-numbered) variable created, and rely on
+        the fact that register reclamation started at higher-numbered registers and worked its
+        way down. So any retained register would block any lower-numbered registers from being
+        reclaimed.
+        
+        Also, preserveLastVar() sets a thing called m_firstConstantIndex. It's unused.
+        
+        This removes preserveLastVar() and makes addVar() retain each register it creates. This
+        is more explicit, since addVar() is the mechanism for creating non-temporary registers.
+        
+        To make this work I had to remove an assertion that Register::setIndex() can only be
+        called when the refcount is zero. This method might be called after a var is created to
+        change its index. This previously worked because preserveLastVar() would be called after
+        we had already made all index changes, so the vars would still have refcount zero. Now
+        they have refcount 1. I think it's OK to lose this assertion; I can't remember this
+        assertion ever firing in a way that alerted me to a serious issue.
+        
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::BytecodeGenerator):
+        (JSC::BytecodeGenerator::preserveLastVar): Deleted.
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::BytecodeGenerator::addVar):
+        * bytecompiler/RegisterID.h:
+        (JSC::RegisterID::setIndex):
+
 2015-02-06  Andreas Kling  <akling@apple.com>
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -152 +152 @@
 }
 
-void BytecodeGenerator::preserveLastVar()
-{
-    if ((m_firstConstantIndex = m_calleeRegisters.size()) != 0)
-        m_lastVar = &m_calleeRegisters.last();
-}
-
 BytecodeGenerator::BytecodeGenerator(VM& vm, ProgramNode* programNode, UnlinkedProgramCodeBlock* codeBlock, DebuggerMode debuggerMode, ProfilerMode profilerMode)
     : m_shouldEmitDebugHooks(Options::forceDebuggerBytecodeGeneration() || debuggerMode == DebuggerOn)
@@ -429 +423 @@
         addParameter(simpleParameter->boundProperty(), index);
     }
-    preserveLastVar();
 
     // We declare the callee's name last because it should lose to a var, function, and/or parameter declaration.
@@ -494 +487 @@
     }
     codeBlock->adoptVariables(variables);
-    preserveLastVar();
 }
 

trunk/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h

@@ -631 +631 @@
         {
             ++m_codeBlock->m_numVars;
-            return newRegister();
+            RegisterID* result = newRegister();
+            ASSERT(VirtualRegister(result->index()).toLocal() == m_codeBlock->m_numVars - 1);
+            result->ref(); // We should never free this slot.
+            return result;
         }
 
@@ -778 +781 @@
         SegmentedVector<Label, 32> m_labels;
         LabelScopeStore m_labelScopes;
-        RefPtr<RegisterID> m_lastVar;
         int m_finallyDepth;
         int m_localScopeDepth;
@@ -792 +794 @@
         SegmentedVector<TryData, 8> m_tryData;
 
-        int m_firstConstantIndex;
         int m_nextConstantOffset;
 

trunk/Source/JavaScriptCore/bytecompiler/RegisterID.h

@@ -71 +71 @@
         void setIndex(int index)
         {
-            ASSERT(!m_refCount);
 #ifndef NDEBUG
             m_didSetIndex = true;