CodeBlock should have a more explicit "strongly referenced" state

https://bugs.webkit.org/show_bug.cgi?id=148714

Reviewed by Filip Pizlo.

Previously, CodeBlock had a "may be executing" bit, which was used by
both the stack visitor and the compiler to indicate "this CodeBlock must
not jettison itself".

Now, CodeBlock has an explicit "is strongly referenced" bit to do the
same.

For now, there is no behavior change. In future, I will use the "is
strongly referenced" bit to indicate the set of all references that
cause a CodeBlock not to jettison itself. Strong references and stack
references will be different because:

    (1) A stack reference requires a write barrier at the end of GC
    (since CodeBlocks only barrier themselves on function entry,
    and GC will clear that barrier); but a strong reference does not
    need or want a write barrier at the end of GC.

    (2) Visiting more heap objects might reveal more strong references
    but, by definition, it cannot reveal more stack references.

Also, this patch adds an explicit mark clearing phase for compiler
CodeBlocks, which does the work that would normally be done by a write
barrier. A compiler CodeBlock can't rely on a normal write barrier
because the compiler writes to CodeBlocks without invoking a write
barrier, and because the CodeBlock write barrier operates on an
executable, but an in-flight compilation is not pointed to by any
executable. This bug does not appear to be noticeable in the current
system, but I will probably make it noticeable.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::CodeBlock):
(JSC::CodeBlock::shouldImmediatelyAssumeLivenessDuringScan):
(JSC::CodeBlock::isKnownToBeLiveDuringGC):
* bytecode/CodeBlock.h:
(JSC::ExecState::uncheckedR):
(JSC::CodeBlockSet::clearMarks):
(JSC::CodeBlockSet::mark):
* dfg/DFGPlan.cpp:
(JSC::DFG::Plan::key):
(JSC::DFG::Plan::clearCodeBlockMarks):
(JSC::DFG::Plan::checkLivenessAndVisitChildren):
* dfg/DFGPlan.h:
* dfg/DFGWorklist.cpp:
(JSC::DFG::Worklist::completeAllPlansForVM):
(JSC::DFG::Worklist::clearCodeBlockMarks):
(JSC::DFG::Worklist::suspendAllThreads):
(JSC::DFG::Worklist::visitWeakReferences):
(JSC::DFG::completeAllPlansForVM):
(JSC::DFG::clearCodeBlockMarks):
* dfg/DFGWorklist.h:
(JSC::DFG::worklistForIndexOrNull):
* heap/CodeBlockSet.cpp:
(JSC::CodeBlockSet::clearMarksForFullCollection):
(JSC::CodeBlockSet::clearMarksForEdenCollection):
(JSC::CodeBlockSet::deleteUnmarkedAndUnreferenced):
(JSC::CodeBlockSet::traceMarked):
(JSC::CodeBlockSet::rememberCurrentlyExecutingCodeBlocks):
* heap/CodeBlockSet.h:
* heap/Heap.cpp:
(JSC::Heap::markRoots):


Canonical link: https://commits.webkit.org/166835@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@189257 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,71 @@
+2015-09-02  Geoffrey Garen  <ggaren@apple.com>
+
+        CodeBlock should have a more explicit "strongly referenced" state
+        https://bugs.webkit.org/show_bug.cgi?id=148714
+
+        Reviewed by Filip Pizlo.
+
+        Previously, CodeBlock had a "may be executing" bit, which was used by
+        both the stack visitor and the compiler to indicate "this CodeBlock must
+        not jettison itself".
+
+        Now, CodeBlock has an explicit "is strongly referenced" bit to do the
+        same.
+
+        For now, there is no behavior change. In future, I will use the "is
+        strongly referenced" bit to indicate the set of all references that
+        cause a CodeBlock not to jettison itself. Strong references and stack
+        references will be different because:
+
+            (1) A stack reference requires a write barrier at the end of GC
+            (since CodeBlocks only barrier themselves on function entry,
+            and GC will clear that barrier); but a strong reference does not
+            need or want a write barrier at the end of GC.
+
+            (2) Visiting more heap objects might reveal more strong references
+            but, by definition, it cannot reveal more stack references.
+
+        Also, this patch adds an explicit mark clearing phase for compiler
+        CodeBlocks, which does the work that would normally be done by a write
+        barrier. A compiler CodeBlock can't rely on a normal write barrier 
+        because the compiler writes to CodeBlocks without invoking a write
+        barrier, and because the CodeBlock write barrier operates on an
+        executable, but an in-flight compilation is not pointed to by any
+        executable. This bug does not appear to be noticeable in the current
+        system, but I will probably make it noticeable.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::CodeBlock):
+        (JSC::CodeBlock::shouldImmediatelyAssumeLivenessDuringScan):
+        (JSC::CodeBlock::isKnownToBeLiveDuringGC):
+        * bytecode/CodeBlock.h:
+        (JSC::ExecState::uncheckedR):
+        (JSC::CodeBlockSet::clearMarks):
+        (JSC::CodeBlockSet::mark):
+        * dfg/DFGPlan.cpp:
+        (JSC::DFG::Plan::key):
+        (JSC::DFG::Plan::clearCodeBlockMarks):
+        (JSC::DFG::Plan::checkLivenessAndVisitChildren):
+        * dfg/DFGPlan.h:
+        * dfg/DFGWorklist.cpp:
+        (JSC::DFG::Worklist::completeAllPlansForVM):
+        (JSC::DFG::Worklist::clearCodeBlockMarks):
+        (JSC::DFG::Worklist::suspendAllThreads):
+        (JSC::DFG::Worklist::visitWeakReferences):
+        (JSC::DFG::completeAllPlansForVM):
+        (JSC::DFG::clearCodeBlockMarks):
+        * dfg/DFGWorklist.h:
+        (JSC::DFG::worklistForIndexOrNull):
+        * heap/CodeBlockSet.cpp:
+        (JSC::CodeBlockSet::clearMarksForFullCollection):
+        (JSC::CodeBlockSet::clearMarksForEdenCollection):
+        (JSC::CodeBlockSet::deleteUnmarkedAndUnreferenced):
+        (JSC::CodeBlockSet::traceMarked):
+        (JSC::CodeBlockSet::rememberCurrentlyExecutingCodeBlocks):
+        * heap/CodeBlockSet.h:
+        * heap/Heap.cpp:
+        (JSC::Heap::markRoots):
+
 2015-09-01  Brian Burg  <bburg@apple.com>
 
         Web Inspector: protocol generator should not allow non-boolean values for "optional" key

Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -1676,6 +1676,7 @@ CodeBlock::CodeBlock(CopyParsedBlockTag, CodeBlock& other)
     , m_isStrictMode(other.m_isStrictMode)
     , m_needsActivation(other.m_needsActivation)
     , m_mayBeExecuting(false)
+    , m_isStronglyReferenced(false)
     , m_source(other.m_source)
     , m_sourceOffset(other.m_sourceOffset)
     , m_firstLineColumnOffset(other.m_firstLineColumnOffset)
@@ -1735,6 +1736,7 @@ CodeBlock::CodeBlock(ScriptExecutable* ownerExecutable, UnlinkedCodeBlock* unlin
     , m_isStrictMode(unlinkedCodeBlock->isStrictMode())
     , m_needsActivation(unlinkedCodeBlock->hasActivationRegister() && unlinkedCodeBlock->codeType() == FunctionCode)
     , m_mayBeExecuting(false)
+    , m_isStronglyReferenced(false)
     , m_source(sourceProvider)
     , m_sourceOffset(sourceOffset)
     , m_firstLineColumnOffset(firstLineColumnOffset)
@@ -2185,6 +2187,7 @@ CodeBlock::CodeBlock(WebAssemblyExecutable* ownerExecutable, VM& vm, JSGlobalObj
     , m_isStrictMode(false)
     , m_needsActivation(false)
     , m_mayBeExecuting(false)
+    , m_isStronglyReferenced(false)
     , m_codeType(FunctionCode)
     , m_osrExitCounter(0)
     , m_optimizationDelayCounter(0)
@@ -2336,10 +2339,7 @@ bool CodeBlock::shouldImmediatelyAssumeLivenessDuringScan()
     if (!JITCode::isOptimizingJIT(jitType()))
         return true;
 
-    // For simplicity, we don't attempt to jettison code blocks during GC if
-    // they are executing. Instead we strongly mark their weak references to
-    // allow them to continue to execute soundly.
-    if (m_mayBeExecuting)
+    if (m_isStronglyReferenced)
         return true;
 
     if (Options::forceDFGCodeBlockLiveness())
@@ -2359,8 +2359,8 @@ bool CodeBlock::isKnownToBeLiveDuringGC()
     //   are live.
     // - Code blocks that were running on the stack.
     // - Code blocks that survived the last GC if the current GC is an Eden GC. This is
-    //   because either livenessHasBeenProved would have survived as true or m_mayBeExecuting
-    //   would survive as true.
+    //   because either livenessHasBeenProved would have survived as true or
+    //   m_isStronglyReferenced would survive as true.
     // - Code blocks that don't have any dead weak references.
 
     return shouldImmediatelyAssumeLivenessDuringScan()

Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -1005,7 +1005,9 @@ class CodeBlock : public ThreadSafeRefCounted<CodeBlock>, public UnconditionalFi
 
     bool m_isStrictMode;
     bool m_needsActivation;
+
     bool m_mayBeExecuting;
+    bool m_isStronglyReferenced;
     Atomic<bool> m_visitAggregateHasBeenCalled;
 
     RefPtr<SourceProvider> m_source;
@@ -1210,6 +1212,16 @@ inline Register& ExecState::uncheckedR(VirtualRegister reg)
     return uncheckedR(reg.offset());
 }
 
+inline void CodeBlockSet::clearMarks(CodeBlock* codeBlock)
+{
+    if (!codeBlock)
+        return;
+
+    codeBlock->m_mayBeExecuting = false;
+    codeBlock->m_isStronglyReferenced = false;
+    codeBlock->m_visitAggregateHasBeenCalled.store(false, std::memory_order_relaxed);
+}
+
 inline void CodeBlockSet::mark(void* candidateCodeBlock)
 {
     // We have to check for 0 and -1 because those are used by the HashMap as markers.
@@ -1237,8 +1249,15 @@ inline void CodeBlockSet::mark(CodeBlock* codeBlock)
         return;
 
     codeBlock->m_mayBeExecuting = true;
+
     // We might not have cleared the marks for this CodeBlock, but we need to visit it.
     codeBlock->m_visitAggregateHasBeenCalled.store(false, std::memory_order_relaxed);
+
+    // For simplicity, we don't attempt to jettison code blocks during GC if
+    // they are executing. Instead we strongly mark their weak references to
+    // allow them to continue to execute soundly.
+    codeBlock->m_isStronglyReferenced = true;
+
 #if ENABLE(GGC)
     m_currentlyExecuting.append(codeBlock);
 #endif

Source/JavaScriptCore/dfg/DFGPlan.cpp

@@ -595,6 +595,13 @@ CompilationKey Plan::key()
     return CompilationKey(codeBlock->alternative(), mode);
 }
 
+void Plan::clearCodeBlockMarks(CodeBlockSet& codeBlocks)
+{
+    codeBlocks.clearMarks(codeBlock.get());
+    codeBlocks.clearMarks(codeBlock->alternative());
+    codeBlocks.clearMarks(profiledDFGCodeBlock.get());
+}
+
 void Plan::checkLivenessAndVisitChildren(SlotVisitor& visitor, CodeBlockSet& codeBlocks)
 {
     if (!isKnownToBeLiveDuringGC())
@@ -603,8 +610,8 @@ void Plan::checkLivenessAndVisitChildren(SlotVisitor& visitor, CodeBlockSet& cod
     for (unsigned i = mustHandleValues.size(); i--;)
         visitor.appendUnbarrieredValue(&mustHandleValues[i]);
 
-    codeBlocks.mark(codeBlock->alternative());
     codeBlocks.mark(codeBlock.get());
+    codeBlocks.mark(codeBlock->alternative());
     codeBlocks.mark(profiledDFGCodeBlock.get());
 
     weakReferences.visitChildren(visitor);

Source/JavaScriptCore/dfg/DFGPlan.h

@@ -72,6 +72,7 @@ struct Plan : public ThreadSafeRefCounted<Plan> {
 
     CompilationKey key();
 
+    void clearCodeBlockMarks(CodeBlockSet&);
     void checkLivenessAndVisitChildren(SlotVisitor&, CodeBlockSet&);
     bool isKnownToBeLiveDuringGC();
     void cancel();

Source/JavaScriptCore/dfg/DFGWorklist.cpp

@@ -207,6 +207,17 @@ void Worklist::completeAllPlansForVM(VM& vm)
     completeAllReadyPlansForVM(vm);
 }
 
+void Worklist::clearCodeBlockMarks(VM& vm, CodeBlockSet& codeBlocks)
+{
+    LockHolder locker(m_lock);
+    for (PlanMap::iterator iter = m_plans.begin(); iter != m_plans.end(); ++iter) {
+        Plan* plan = iter->value.get();
+        if (&plan->vm != &vm)
+            continue;
+        plan->clearCodeBlockMarks(codeBlocks);
+    }
+}
+
 void Worklist::suspendAllThreads()
 {
     m_suspensionLock.lock();
@@ -230,7 +241,7 @@ void Worklist::visitWeakReferences(SlotVisitor& visitor, CodeBlockSet& codeBlock
             Plan* plan = iter->value.get();
             if (&plan->vm != vm)
                 continue;
-            iter->value->checkLivenessAndVisitChildren(visitor, codeBlocks);
+            plan->checkLivenessAndVisitChildren(visitor, codeBlocks);
         }
     }
     // This loop doesn't need locking because:
@@ -456,6 +467,14 @@ void completeAllPlansForVM(VM& vm)
     }
 }
 
+void clearCodeBlockMarks(VM& vm, CodeBlockSet& codeBlockSet)
+{
+    for (unsigned i = DFG::numberOfWorklists(); i--;) {
+        if (DFG::Worklist* worklist = DFG::worklistForIndexOrNull(i))
+            worklist->clearCodeBlockMarks(vm, codeBlockSet);
+    }
+}
+
 } } // namespace JSC::DFG
 
 #endif // ENABLE(DFG_JIT)

Source/JavaScriptCore/dfg/DFGWorklist.h

@@ -57,7 +57,9 @@ class Worklist : public RefCounted<Worklist> {
     // worklist->waitUntilAllPlansForVMAreReady(vm);
     // worklist->completeAllReadyPlansForVM(vm);
     void completeAllPlansForVM(VM&);
-
+
+    void clearCodeBlockMarks(VM&, CodeBlockSet&);
+
     void waitUntilAllPlansForVMAreReady(VM&);
     State completeAllReadyPlansForVM(VM&, CompilationKey = CompilationKey());
     void removeAllReadyPlansForVM(VM&);
@@ -140,6 +142,7 @@ inline Worklist* worklistForIndexOrNull(unsigned index)
 }
 
 void completeAllPlansForVM(VM&);
+void clearCodeBlockMarks(VM&, CodeBlockSet&);
 
 } } // namespace JSC::DFG
 

Source/JavaScriptCore/heap/CodeBlockSet.cpp

@@ -63,10 +63,8 @@ void CodeBlockSet::promoteYoungCodeBlocks()
 
 void CodeBlockSet::clearMarksForFullCollection()
 {
-    for (CodeBlock* codeBlock : m_oldCodeBlocks) {
-        codeBlock->m_mayBeExecuting = false;
-        codeBlock->m_visitAggregateHasBeenCalled.store(false, std::memory_order_relaxed);
-    }
+    for (CodeBlock* codeBlock : m_oldCodeBlocks)
+        clearMarks(codeBlock);
 
     // We promote after we clear marks on the old generation CodeBlocks because
     // none of the young generations CodeBlocks need to be cleared.
@@ -80,9 +78,8 @@ void CodeBlockSet::clearMarksForEdenCollection(const Vector<const JSCell*>& reme
         ScriptExecutable* executable = const_cast<ScriptExecutable*>(jsDynamicCast<const ScriptExecutable*>(cell));
         if (!executable)
             continue;
-        executable->forEachCodeBlock([](CodeBlock* codeBlock) {
-            codeBlock->m_mayBeExecuting = false;
-            codeBlock->m_visitAggregateHasBeenCalled.store(false, std::memory_order_relaxed);
+        executable->forEachCodeBlock([this](CodeBlock* codeBlock) {
+            clearMarks(codeBlock);
         });
     }
 }
@@ -102,7 +99,7 @@ void CodeBlockSet::deleteUnmarkedAndUnreferenced(HeapOperation collectionType)
         for (CodeBlock* codeBlock : set) {
             if (!codeBlock->hasOneRef())
                 continue;
-            if (codeBlock->m_mayBeExecuting)
+            if (codeBlock->m_isStronglyReferenced)
                 continue;
             codeBlock->deref();
             toRemove.append(codeBlock);
@@ -138,6 +135,7 @@ void CodeBlockSet::traceMarked(SlotVisitor& visitor)
         dataLog("Tracing ", m_currentlyExecuting.size(), " code blocks.\n");
     for (CodeBlock* codeBlock : m_currentlyExecuting) {
         ASSERT(codeBlock->m_mayBeExecuting);
+        ASSERT(codeBlock->m_isStronglyReferenced);
         codeBlock->visitAggregate(visitor);
     }
 }
@@ -148,8 +146,9 @@ void CodeBlockSet::rememberCurrentlyExecutingCodeBlocks(Heap* heap)
     if (verbose)
         dataLog("Remembering ", m_currentlyExecuting.size(), " code blocks.\n");
     for (CodeBlock* codeBlock : m_currentlyExecuting) {
-        heap->addToRememberedSet(codeBlock->ownerExecutable());
         ASSERT(codeBlock->m_mayBeExecuting);
+        ASSERT(codeBlock->m_isStronglyReferenced);
+        heap->addToRememberedSet(codeBlock->ownerExecutable());
     }
     m_currentlyExecuting.clear();
 #else

Source/JavaScriptCore/heap/CodeBlockSet.h

@@ -61,6 +61,8 @@ class CodeBlockSet {
     // Clear all mark bits for all CodeBlocks.
     void clearMarksForFullCollection();
 
+    void clearMarks(CodeBlock*);
+
     // Mark a pointer that may be a CodeBlock that belongs to the set of DFG
     // blocks. This is defined in CodeBlock.h.
     void mark(CodeBlock* candidateCodeBlock);

Source/JavaScriptCore/heap/Heap.cpp

@@ -538,6 +538,7 @@ void Heap::markRoots(double gcStartTime, void* stackOrigin, void* stackTop, Mach
     Vector<const JSCell*> rememberedSet;
 #endif
 
+    DFG::clearCodeBlockMarks(*m_vm, m_codeBlocks);
     if (m_operationInProgress == EdenCollection)
         m_codeBlocks.clearMarksForEdenCollection(rememberedSet);
     else