Make opaque root scanning truly constraint-based
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Dec 2016 19:54:15 +0000 (19:54 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 13 Dec 2016 19:54:15 +0000 (19:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=165760

Reviewed by Saam Barati.
Source/JavaScriptCore:

We have bugs when visitChildren() changes its mind about what opaque root to add, since
we don't have barriers on opaque roots. This supposedly once worked for generational GC,
and I started adding more barriers to support concurrent GC. But I think that the real
bug here is that we want the JSObject->OpaqueRoot to be evaluated as a constraint that
participates in the fixpoint. A constraint is different from the normal visiting in that
the GC will not wait for a barrier to rescan the object.

So, it's now possible for any visitChildren() method to become a constraint by calling
slotVisitor.rescanAsConstraint(). Because opaque roots are constraints, addOpaqueRoot()
does rescanAsConstraint() for you.

The constraint set is simply a HashSet<JSCell*> that accumulates with every
rescanAsConstraint() call and is only cleared at the start of full GC. This trivially
resolves most classes of GC bugs that would have arisen from opaque roots being changed
in a way that the GC did not anticipate.

Looks like this is perf-neutral.

* heap/Heap.cpp:
(JSC::Heap::markToFixpoint):
(JSC::Heap::setMutatorShouldBeFenced):
(JSC::Heap::writeBarrierOpaqueRootSlow): Deleted.
(JSC::Heap::addMutatorShouldBeFencedCache): Deleted.
* heap/Heap.h:
* heap/HeapInlines.h:
(JSC::Heap::writeBarrierOpaqueRoot): Deleted.
* heap/MarkedSpace.cpp:
(JSC::MarkedSpace::visitWeakSets):
* heap/MarkedSpace.h:
* heap/SlotVisitor.cpp:
(JSC::SlotVisitor::visitChildren):
(JSC::SlotVisitor::visitSubsequently):
(JSC::SlotVisitor::drain):
(JSC::SlotVisitor::addOpaqueRoot):
(JSC::SlotVisitor::rescanAsConstraint):
(JSC::SlotVisitor::mergeIfNecessary):
(JSC::SlotVisitor::mergeOpaqueRootsAndConstraints):
(JSC::SlotVisitor::mergeOpaqueRootsIfNecessary): Deleted.
* heap/SlotVisitor.h:
* heap/SlotVisitorInlines.h:
(JSC::SlotVisitor::reportExtraMemoryVisited):
(JSC::SlotVisitor::reportExternalMemoryVisited):
(JSC::SlotVisitor::didNotRace):
* heap/WeakBlock.cpp:
(JSC::WeakBlock::specializedVisit):
(JSC::WeakBlock::visit):
* heap/WeakBlock.h:
* heap/WeakSet.h:
(JSC::WeakSet::visit):

Source/WebCore:

No new tests yet. I think that writing tests for this is a big investigation:
https://bugs.webkit.org/show_bug.cgi?id=165808

Remove the previous advancing wavefront DOM write barrier. I don't think this will scale
very well. It's super confusing.

This change makes it so that visitChildren can become a GC constraint that executes as
part of the fixpoint. This changes all WebCore visitChildren methods that do opaque
roots into constraints.

* bindings/js/CommonVM.cpp:
(WebCore::commonVMSlow):
(WebCore::writeBarrierOpaqueRootSlow): Deleted.
* bindings/js/CommonVM.h:
(WebCore::writeBarrierOpaqueRoot): Deleted.
* bindings/js/JSAttrCustom.cpp:
(WebCore::JSAttr::visitAdditionalChildren):
* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSDOMWindow::visitAdditionalChildren):
* bindings/js/JSIDBCursorCustom.cpp:
(WebCore::JSIDBCursor::visitAdditionalChildren):
* bindings/js/JSMessageChannelCustom.cpp:
(WebCore::JSMessageChannel::visitAdditionalChildren):
* bindings/js/JSMessagePortCustom.cpp:
(WebCore::JSMessagePort::visitAdditionalChildren):
* bindings/js/JSNodeIteratorCustom.cpp:
(WebCore::JSNodeIterator::visitAdditionalChildren):
* bindings/js/JSTextTrackCueCustom.cpp:
(WebCore::JSTextTrackCue::visitAdditionalChildren):
* bindings/js/JSTreeWalkerCustom.cpp:
(WebCore::JSTreeWalker::visitAdditionalChildren):
* bindings/js/JSWorkerGlobalScopeCustom.cpp:
(WebCore::JSWorkerGlobalScope::visitAdditionalChildren):
* bindings/js/JSXMLHttpRequestCustom.cpp:
(WebCore::JSXMLHttpRequest::visitAdditionalChildren):
* bindings/js/JSXPathResultCustom.cpp:
(WebCore::JSXPathResult::visitAdditionalChildren):
* dom/ContainerNodeAlgorithms.cpp:
(WebCore::notifyChildNodeInserted):
(WebCore::notifyChildNodeRemoved):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@209766 268f45cc-cd09-0410-ab3c-d52691b4dbfc

27 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/HeapInlines.h
Source/JavaScriptCore/heap/MarkedSpace.cpp
Source/JavaScriptCore/heap/MarkedSpace.h
Source/JavaScriptCore/heap/SlotVisitor.cpp
Source/JavaScriptCore/heap/SlotVisitor.h
Source/JavaScriptCore/heap/SlotVisitorInlines.h
Source/JavaScriptCore/heap/WeakBlock.cpp
Source/JavaScriptCore/heap/WeakBlock.h
Source/JavaScriptCore/heap/WeakSet.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/CommonVM.cpp
Source/WebCore/bindings/js/CommonVM.h
Source/WebCore/bindings/js/JSAttrCustom.cpp
Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
Source/WebCore/bindings/js/JSIDBCursorCustom.cpp
Source/WebCore/bindings/js/JSMessageChannelCustom.cpp
Source/WebCore/bindings/js/JSMessagePortCustom.cpp
Source/WebCore/bindings/js/JSNodeIteratorCustom.cpp
Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp
Source/WebCore/bindings/js/JSTreeWalkerCustom.cpp
Source/WebCore/bindings/js/JSWorkerGlobalScopeCustom.cpp
Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp
Source/WebCore/bindings/js/JSXPathResultCustom.cpp
Source/WebCore/dom/ContainerNodeAlgorithms.cpp

index e431dab..8c8000c 100644 (file)
@@ -1,3 +1,60 @@
+2016-12-13  Filip Pizlo  <fpizlo@apple.com>
+
+        Make opaque root scanning truly constraint-based
+        https://bugs.webkit.org/show_bug.cgi?id=165760
+
+        Reviewed by Saam Barati.
+        
+        We have bugs when visitChildren() changes its mind about what opaque root to add, since
+        we don't have barriers on opaque roots. This supposedly once worked for generational GC,
+        and I started adding more barriers to support concurrent GC. But I think that the real
+        bug here is that we want the JSObject->OpaqueRoot to be evaluated as a constraint that
+        participates in the fixpoint. A constraint is different from the normal visiting in that
+        the GC will not wait for a barrier to rescan the object.
+        
+        So, it's now possible for any visitChildren() method to become a constraint by calling
+        slotVisitor.rescanAsConstraint(). Because opaque roots are constraints, addOpaqueRoot()
+        does rescanAsConstraint() for you.
+        
+        The constraint set is simply a HashSet<JSCell*> that accumulates with every
+        rescanAsConstraint() call and is only cleared at the start of full GC. This trivially
+        resolves most classes of GC bugs that would have arisen from opaque roots being changed
+        in a way that the GC did not anticipate.
+        
+        Looks like this is perf-neutral.
+        
+        * heap/Heap.cpp:
+        (JSC::Heap::markToFixpoint):
+        (JSC::Heap::setMutatorShouldBeFenced):
+        (JSC::Heap::writeBarrierOpaqueRootSlow): Deleted.
+        (JSC::Heap::addMutatorShouldBeFencedCache): Deleted.
+        * heap/Heap.h:
+        * heap/HeapInlines.h:
+        (JSC::Heap::writeBarrierOpaqueRoot): Deleted.
+        * heap/MarkedSpace.cpp:
+        (JSC::MarkedSpace::visitWeakSets):
+        * heap/MarkedSpace.h:
+        * heap/SlotVisitor.cpp:
+        (JSC::SlotVisitor::visitChildren):
+        (JSC::SlotVisitor::visitSubsequently):
+        (JSC::SlotVisitor::drain):
+        (JSC::SlotVisitor::addOpaqueRoot):
+        (JSC::SlotVisitor::rescanAsConstraint):
+        (JSC::SlotVisitor::mergeIfNecessary):
+        (JSC::SlotVisitor::mergeOpaqueRootsAndConstraints):
+        (JSC::SlotVisitor::mergeOpaqueRootsIfNecessary): Deleted.
+        * heap/SlotVisitor.h:
+        * heap/SlotVisitorInlines.h:
+        (JSC::SlotVisitor::reportExtraMemoryVisited):
+        (JSC::SlotVisitor::reportExternalMemoryVisited):
+        (JSC::SlotVisitor::didNotRace):
+        * heap/WeakBlock.cpp:
+        (JSC::WeakBlock::specializedVisit):
+        (JSC::WeakBlock::visit):
+        * heap/WeakBlock.h:
+        * heap/WeakSet.h:
+        (JSC::WeakSet::visit):
+
 2016-12-13  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r209725.
index d6165aa..d39856d 100644 (file)
@@ -519,6 +519,7 @@ void Heap::markToFixpoint(double gcStartTime)
     
     if (m_collectionScope == CollectionScope::Full) {
         m_opaqueRoots.clear();
+        m_constraints.clear();
         m_collectorSlotVisitor->clearMarkStacks();
         m_mutatorMarkStack->clear();
     }
@@ -617,18 +618,22 @@ void Heap::markToFixpoint(double gcStartTime)
                 
         m_jitStubRoutines->traceMarkedStubRoutines(*m_collectorSlotVisitor);
 
-        m_collectorSlotVisitor->mergeOpaqueRootsIfNecessary();
+        m_collectorSlotVisitor->mergeIfNecessary();
         for (auto& parallelVisitor : m_parallelSlotVisitors)
-            parallelVisitor->mergeOpaqueRootsIfNecessary();
+            parallelVisitor->mergeIfNecessary();
+        
+        for (JSCell* cell : m_constraints)
+            m_collectorSlotVisitor->visitSubsequently(cell);
+        m_collectorSlotVisitor->mergeIfNecessary();
 
-        m_objectSpace.visitWeakSets(heapRootVisitor);
+        size_t weakSetCount = m_objectSpace.visitWeakSets(heapRootVisitor);
         harvestWeakReferences();
         visitCompilerWorklistWeakReferences();
         DFG::markCodeBlocks(*m_vm, *m_collectorSlotVisitor);
         bool shouldTerminate = m_collectorSlotVisitor->isEmpty() && m_mutatorMarkStack->isEmpty();
         
         if (Options::logGC()) {
-            dataLog(m_collectorSlotVisitor->collectorMarkStack().size(), "+", m_mutatorMarkStack->size() + m_collectorSlotVisitor->mutatorMarkStack().size(), ", a=", m_bytesAllocatedThisCycle / 1024, " kb, b=", m_barriersExecuted, ", mu=", scheduler.currentDecision().targetMutatorUtilization(), " ");
+            dataLog(m_collectorSlotVisitor->collectorMarkStack().size(), "+", m_mutatorMarkStack->size() + m_collectorSlotVisitor->mutatorMarkStack().size(), ", a=", m_bytesAllocatedThisCycle / 1024, " kb, b=", m_barriersExecuted, ", mu=", scheduler.currentDecision().targetMutatorUtilization(), ", ws=", weakSetCount, ", cs=", m_constraints.size(), " ");
         }
         
         // We want to do this to conservatively ensure that we rescan any code blocks that are
@@ -2220,27 +2225,10 @@ void Heap::forEachSlotVisitor(const Func& func)
         func(*slotVisitor);
 }
 
-void Heap::writeBarrierOpaqueRootSlow(void* root)
-{
-    ASSERT(mutatorShouldBeFenced());
-    
-    auto locker = holdLock(m_opaqueRootsMutex);
-    m_opaqueRoots.add(root);
-}
-
-void Heap::addMutatorShouldBeFencedCache(bool& cache)
-{
-    ASSERT(hasHeapAccess());
-    cache = m_mutatorShouldBeFenced;
-    m_mutatorShouldBeFencedCaches.append(&cache);
-}
-
 void Heap::setMutatorShouldBeFenced(bool value)
 {
     m_mutatorShouldBeFenced = value;
     m_barrierThreshold = value ? tautologicalThreshold : blackThreshold;
-    for (bool* cache : m_mutatorShouldBeFencedCaches)
-        *cache = value;
 }
     
 } // namespace JSC
index 3e0ce66..dd4cfec 100644 (file)
@@ -127,8 +127,6 @@ public:
     WriteBarrierBuffer& writeBarrierBuffer() { return m_writeBarrierBuffer; }
     void flushWriteBarrierBuffer(JSCell*);
     
-    void writeBarrierOpaqueRoot(void*);
-
     Heap(VM*, HeapType);
     ~Heap();
     void lastChanceToFinalize();
@@ -352,8 +350,6 @@ public:
     void preventCollection();
     void allowCollection();
     
-    JS_EXPORT_PRIVATE void addMutatorShouldBeFencedCache(bool&);
-    
 #if USE(CF)
     CFRunLoopRef runLoop() const { return m_runLoop.get(); }
     JS_EXPORT_PRIVATE void setRunLoop(CFRunLoopRef);
@@ -497,8 +493,6 @@ private:
     
     void forEachCodeBlockImpl(const ScopedLambda<bool(CodeBlock*)>&);
     
-    JS_EXPORT_PRIVATE void writeBarrierOpaqueRootSlow(void*);
-    
     void setMutatorShouldBeFenced(bool value);
 
     const HeapType m_heapType;
@@ -559,7 +553,6 @@ private:
     WriteBarrierBuffer m_writeBarrierBuffer;
     bool m_mutatorShouldBeFenced { Options::forceFencedBarrier() };
     unsigned m_barrierThreshold { Options::forceFencedBarrier() ? tautologicalThreshold : blackThreshold };
-    Vector<bool*> m_mutatorShouldBeFencedCaches;
 
     VM* m_vm;
     double m_lastFullGCLength;
@@ -603,7 +596,8 @@ private:
     bool m_parallelMarkersShouldExit { false };
 
     Lock m_opaqueRootsMutex;
-    HashSet<void*> m_opaqueRoots;
+    HashSet<const void*> m_opaqueRoots;
+    HashSet<JSCell*> m_constraints;
 
     static const size_t s_blockFragmentLength = 32;
 
index 55589ff..430a90e 100644 (file)
@@ -370,10 +370,4 @@ inline void Heap::stopIfNecessary()
     stopIfNecessarySlow();
 }
 
-inline void Heap::writeBarrierOpaqueRoot(void* root)
-{
-    if (mutatorShouldBeFenced())
-        writeBarrierOpaqueRootSlow(root);
-}
-
 } // namespace JSC
index 7b81944..8f42485 100644 (file)
@@ -359,16 +359,18 @@ void MarkedSpace::prepareForAllocation()
     m_allocatorForEmptyAllocation = m_firstAllocator;
 }
 
-void MarkedSpace::visitWeakSets(HeapRootVisitor& heapRootVisitor)
+size_t MarkedSpace::visitWeakSets(HeapRootVisitor& heapRootVisitor)
 {
+    size_t count = 0;
     auto visit = [&] (WeakSet* weakSet) {
-        weakSet->visit(heapRootVisitor);
+        count += weakSet->visit(heapRootVisitor);
     };
     
     m_newActiveWeakSets.forEach(visit);
     
     if (m_heap->collectionScope() == CollectionScope::Full)
         m_activeWeakSets.forEach(visit);
+    return count;
 }
 
 void MarkedSpace::reapWeakSets()
index c61aa59..f444e59 100644 (file)
@@ -129,7 +129,7 @@ public:
     
     void prepareForAllocation();
 
-    void visitWeakSets(HeapRootVisitor&);
+    size_t visitWeakSets(HeapRootVisitor&);
     void reapWeakSets();
 
     MarkedBlockSet& blocks() { return m_blocks; }
index bbf2e34..3278aef 100644 (file)
@@ -356,8 +356,8 @@ ALWAYS_INLINE void SlotVisitor::visitChildren(const JSCell* cell)
     
     if (false) {
         dataLog("Visiting ", RawPointer(cell));
-        if (m_isVisitingMutatorStack)
-            dataLog(" (mutator)");
+        if (!m_isFirstVisit)
+            dataLog(" (subsequent)");
         dataLog("\n");
     }
     
@@ -391,11 +391,17 @@ ALWAYS_INLINE void SlotVisitor::visitChildren(const JSCell* cell)
     }
     
     if (UNLIKELY(m_heapSnapshotBuilder)) {
-        if (!m_isVisitingMutatorStack)
+        if (m_isFirstVisit)
             m_heapSnapshotBuilder->appendNode(const_cast<JSCell*>(cell));
     }
 }
 
+void SlotVisitor::visitSubsequently(JSCell* cell)
+{
+    m_isFirstVisit = false;
+    visitChildren(cell);
+}
+
 void SlotVisitor::donateKnownParallel(MarkStackArray& from, MarkStackArray& to)
 {
     // NOTE: Because we re-try often, we can afford to be conservative, and
@@ -465,7 +471,7 @@ void SlotVisitor::drain(MonotonicTime timeout)
         updateMutatorIsStopped(locker);
         if (!m_collectorStack.isEmpty()) {
             m_collectorStack.refill();
-            m_isVisitingMutatorStack = false;
+            m_isFirstVisit = true;
             for (unsigned countdown = Options::minimumNumberOfScansBetweenRebalance(); m_collectorStack.canRemoveLast() && countdown--;)
                 visitChildren(m_collectorStack.removeLast());
         } else if (!m_mutatorStack.isEmpty()) {
@@ -473,7 +479,7 @@ void SlotVisitor::drain(MonotonicTime timeout)
             // We know for sure that we are visiting objects because of the barrier, not because of
             // marking. Marking will visit an object exactly once. The barrier will visit it
             // possibly many times, and always after it was already marked.
-            m_isVisitingMutatorStack = true;
+            m_isFirstVisit = false;
             for (unsigned countdown = Options::minimumNumberOfScansBetweenRebalance(); m_mutatorStack.canRemoveLast() && countdown--;)
                 visitChildren(m_mutatorStack.removeLast());
         }
@@ -481,7 +487,7 @@ void SlotVisitor::drain(MonotonicTime timeout)
         donateKnownParallel();
     }
     
-    mergeOpaqueRootsIfNecessary();
+    mergeIfNecessary();
 }
 
 bool SlotVisitor::didReachTermination()
@@ -597,12 +603,24 @@ void SlotVisitor::addOpaqueRoot(void* root)
     if (Options::numberOfGCMarkers() == 1) {
         // Put directly into the shared HashSet.
         m_heap.m_opaqueRoots.add(root);
+        m_heap.m_constraints.add(m_currentCell);
         return;
     }
     // Put into the local set, but merge with the shared one every once in
     // a while to make sure that the local sets don't grow too large.
     mergeOpaqueRootsIfProfitable();
     m_opaqueRoots.add(root);
+    m_constraints.add(m_currentCell);
+}
+
+void SlotVisitor::rescanAsConstraint()
+{
+    if (Options::numberOfGCMarkers() == 1) {
+        m_heap.m_constraints.add(m_currentCell);
+        return;
+    }
+    
+    m_constraints.add(m_currentCell);
 }
 
 bool SlotVisitor::containsOpaqueRoot(void* root) const
@@ -621,13 +639,13 @@ TriState SlotVisitor::containsOpaqueRootTriState(void* root) const
     return MixedTriState;
 }
 
-void SlotVisitor::mergeOpaqueRootsIfNecessary()
+void SlotVisitor::mergeIfNecessary()
 {
-    if (m_opaqueRoots.isEmpty())
+    if (m_opaqueRoots.isEmpty() && m_constraints.isEmpty())
         return;
-    mergeOpaqueRoots();
+    mergeOpaqueRootsAndConstraints();
 }
-    
+
 void SlotVisitor::mergeOpaqueRootsIfProfitable()
 {
     if (static_cast<unsigned>(m_opaqueRoots.size()) < Options::opaqueRootMergeThreshold())
@@ -660,6 +678,19 @@ void SlotVisitor::mergeOpaqueRoots()
     m_opaqueRoots.clear();
 }
 
+void SlotVisitor::mergeOpaqueRootsAndConstraints()
+{
+    {
+        std::lock_guard<Lock> lock(m_heap.m_opaqueRootsMutex);
+        for (const void* root : m_opaqueRoots)
+            m_heap.m_opaqueRoots.add(root);
+        for (JSCell* constraint : m_constraints)
+            m_heap.m_constraints.add(constraint);
+    }
+    m_opaqueRoots.clear();
+    m_constraints.clear();
+}
+
 void SlotVisitor::addWeakReferenceHarvester(WeakReferenceHarvester* weakReferenceHarvester)
 {
     m_heap.m_weakReferenceHarvesters.addThreadSafe(weakReferenceHarvester);
index b4d2f80..04baacf 100644 (file)
@@ -85,7 +85,18 @@ public:
     void appendUnbarrieredReadOnlyPointer(T*);
     void appendUnbarrieredReadOnlyValue(JSValue);
     
+    void visitSubsequently(JSCell*);
+    
+    // Does your visitChildren do logic that depends on non-JS-object state that can
+    // change during the course of a GC, or in between GCs? Then you should call this
+    // method! It will cause the GC to invoke your visitChildren method again just before
+    // terminating with the world stopped.
+    JS_EXPORT_PRIVATE void rescanAsConstraint();
+    
+    // Implies rescanAsConstraint, so you don't have to call rescanAsConstraint() if you
+    // call this unconditionally.
     JS_EXPORT_PRIVATE void addOpaqueRoot(void*);
+    
     JS_EXPORT_PRIVATE bool containsOpaqueRoot(void*) const;
     TriState containsOpaqueRootTriState(void*) const;
 
@@ -108,6 +119,8 @@ public:
 
     SharedDrainResult drainInParallel(MonotonicTime timeout = MonotonicTime::infinity());
     SharedDrainResult drainInParallelPassively(MonotonicTime timeout = MonotonicTime::infinity());
+    
+    void mergeIfNecessary();
 
     // This informs the GC about auxiliary of some size that we are keeping alive. If you don't do
     // this then the space will be freed at end of GC.
@@ -127,8 +140,6 @@ public:
     
     HeapVersion markingVersion() const { return m_markingVersion; }
 
-    void mergeOpaqueRootsIfNecessary();
-    
     bool mutatorIsStopped() const { return m_mutatorIsStopped; }
     
     Lock& rightToRun() { return m_rightToRun; }
@@ -167,7 +178,9 @@ private:
     
     void noteLiveAuxiliaryCell(HeapCell*);
     
-    JS_EXPORT_PRIVATE void mergeOpaqueRoots();
+    void mergeOpaqueRoots();
+    void mergeOpaqueRootsAndConstraints();
+
     void mergeOpaqueRootsIfProfitable();
 
     void visitChildren(const JSCell*);
@@ -181,6 +194,7 @@ private:
     MarkStackArray m_collectorStack;
     MarkStackArray m_mutatorStack;
     OpaqueRootSet m_opaqueRoots; // Handle-owning data structures not visible to the garbage collector.
+    HashSet<JSCell*> m_constraints;
     
     size_t m_bytesVisited;
     size_t m_visitCount;
@@ -192,7 +206,7 @@ private:
 
     HeapSnapshotBuilder* m_heapSnapshotBuilder { nullptr };
     JSCell* m_currentCell { nullptr };
-    bool m_isVisitingMutatorStack { false };
+    bool m_isFirstVisit { false };
     bool m_mutatorIsStopped { false };
     bool m_canOptimizeForStoppedMutator { false };
     Lock m_rightToRun;
index cf335d0..9adb1e7 100644 (file)
@@ -95,14 +95,14 @@ inline void SlotVisitor::appendValuesHidden(WriteBarrierBase<Unknown>* barriers,
 
 inline void SlotVisitor::reportExtraMemoryVisited(size_t size)
 {
-    if (!m_isVisitingMutatorStack)
+    if (m_isFirstVisit)
         heap()->reportExtraMemoryVisited(size);
 }
 
 #if ENABLE(RESOURCE_USAGE)
 inline void SlotVisitor::reportExternalMemoryVisited(size_t size)
 {
-    if (!m_isVisitingMutatorStack)
+    if (m_isFirstVisit)
         heap()->reportExternalMemoryVisited(size);
 }
 #endif
@@ -127,7 +127,7 @@ inline void SlotVisitor::didNotRace(const VisitRaceKey& race)
     if (ASSERT_DISABLED)
         return;
     
-    if (!m_isVisitingMutatorStack) {
+    if (m_isFirstVisit) {
         // This is the first visit so we don't need to remove anything.
         return;
     }
index dc57cc4..475d84c 100644 (file)
@@ -97,13 +97,14 @@ void WeakBlock::sweep()
 }
 
 template<typename ContainerType>
-void WeakBlock::specializedVisit(ContainerType& container, HeapRootVisitor& heapRootVisitor)
+size_t WeakBlock::specializedVisit(ContainerType& container, HeapRootVisitor& heapRootVisitor)
 {
     SlotVisitor& visitor = heapRootVisitor.visitor();
     
     HeapVersion markingVersion = visitor.markingVersion();
 
-    for (size_t i = 0; i < weakImplCount(); ++i) {
+    size_t count = weakImplCount();
+    for (size_t i = 0; i < count; ++i) {
         WeakImpl* weakImpl = &weakImpls()[i];
         if (weakImpl->state() != WeakImpl::Live)
             continue;
@@ -121,21 +122,22 @@ void WeakBlock::specializedVisit(ContainerType& container, HeapRootVisitor& heap
 
         heapRootVisitor.visit(&const_cast<JSValue&>(jsValue));
     }
+    
+    return count;
 }
 
-void WeakBlock::visit(HeapRootVisitor& heapRootVisitor)
+size_t WeakBlock::visit(HeapRootVisitor& heapRootVisitor)
 {
     // If a block is completely empty, a visit won't have any effect.
     if (isEmpty())
-        return;
+        return 0;
 
     // If this WeakBlock doesn't belong to a CellContainer, we won't even be here.
     ASSERT(m_container);
     
     if (m_container.isLargeAllocation())
-        specializedVisit(m_container.largeAllocation(), heapRootVisitor);
-    else
-        specializedVisit(m_container.markedBlock(), heapRootVisitor);
+        return specializedVisit(m_container.largeAllocation(), heapRootVisitor);
+    return specializedVisit(m_container.markedBlock(), heapRootVisitor);
 }
 
 void WeakBlock::reap()
index b6453f6..bd58ea8 100644 (file)
@@ -63,7 +63,7 @@ public:
     void sweep();
     SweepResult takeSweepResult();
 
-    void visit(HeapRootVisitor&);
+    size_t visit(HeapRootVisitor&);
     void reap();
 
     void lastChanceToFinalize();
@@ -73,7 +73,7 @@ private:
     static FreeCell* asFreeCell(WeakImpl*);
     
     template<typename ContainerType>
-    void specializedVisit(ContainerType&, HeapRootVisitor&);
+    size_t specializedVisit(ContainerType&, HeapRootVisitor&);
 
     explicit WeakBlock(CellContainer);
     void finalize(WeakImpl*);
index 36fb594..47f9d86 100644 (file)
@@ -53,7 +53,7 @@ public:
 
     bool isEmpty() const;
 
-    unsigned visit(HeapRootVisitor&);
+    size_t visit(HeapRootVisitor&);
     void reap();
     void sweep();
     void shrink();
@@ -106,13 +106,11 @@ inline void WeakSet::lastChanceToFinalize()
         block->lastChanceToFinalize();
 }
 
-inline unsigned WeakSet::visit(HeapRootVisitor& visitor)
+inline size_t WeakSet::visit(HeapRootVisitor& visitor)
 {
-    unsigned count = 0;
-    for (WeakBlock* block = m_blocks.head(); block; block = block->next()) {
-        count++;
-        block->visit(visitor);
-    }
+    size_t count = 0;
+    for (WeakBlock* block = m_blocks.head(); block; block = block->next())
+        count += block->visit(visitor);
     return count;
 }
 
index 41d5c99..29f3fd7 100644 (file)
@@ -1,3 +1,51 @@
+2016-12-13  Filip Pizlo  <fpizlo@apple.com>
+
+        Make opaque root scanning truly constraint-based
+        https://bugs.webkit.org/show_bug.cgi?id=165760
+
+        Reviewed by Saam Barati.
+
+        No new tests yet. I think that writing tests for this is a big investigation:
+        https://bugs.webkit.org/show_bug.cgi?id=165808
+        
+        Remove the previous advancing wavefront DOM write barrier. I don't think this will scale
+        very well. It's super confusing.
+        
+        This change makes it so that visitChildren can become a GC constraint that executes as
+        part of the fixpoint. This changes all WebCore visitChildren methods that do opaque
+        roots into constraints.
+
+        * bindings/js/CommonVM.cpp:
+        (WebCore::commonVMSlow):
+        (WebCore::writeBarrierOpaqueRootSlow): Deleted.
+        * bindings/js/CommonVM.h:
+        (WebCore::writeBarrierOpaqueRoot): Deleted.
+        * bindings/js/JSAttrCustom.cpp:
+        (WebCore::JSAttr::visitAdditionalChildren):
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::JSDOMWindow::visitAdditionalChildren):
+        * bindings/js/JSIDBCursorCustom.cpp:
+        (WebCore::JSIDBCursor::visitAdditionalChildren):
+        * bindings/js/JSMessageChannelCustom.cpp:
+        (WebCore::JSMessageChannel::visitAdditionalChildren):
+        * bindings/js/JSMessagePortCustom.cpp:
+        (WebCore::JSMessagePort::visitAdditionalChildren):
+        * bindings/js/JSNodeIteratorCustom.cpp:
+        (WebCore::JSNodeIterator::visitAdditionalChildren):
+        * bindings/js/JSTextTrackCueCustom.cpp:
+        (WebCore::JSTextTrackCue::visitAdditionalChildren):
+        * bindings/js/JSTreeWalkerCustom.cpp:
+        (WebCore::JSTreeWalker::visitAdditionalChildren):
+        * bindings/js/JSWorkerGlobalScopeCustom.cpp:
+        (WebCore::JSWorkerGlobalScope::visitAdditionalChildren):
+        * bindings/js/JSXMLHttpRequestCustom.cpp:
+        (WebCore::JSXMLHttpRequest::visitAdditionalChildren):
+        * bindings/js/JSXPathResultCustom.cpp:
+        (WebCore::JSXPathResult::visitAdditionalChildren):
+        * dom/ContainerNodeAlgorithms.cpp:
+        (WebCore::notifyChildNodeInserted):
+        (WebCore::notifyChildNodeRemoved):
+
 2016-12-12  Sam Weinig  <sam@webkit.org>
 
         [WebIDL] Remove use of Dictionary in ApplePaySession
index 86f7060..adda7f4 100644 (file)
@@ -38,7 +38,6 @@ using namespace JSC;
 namespace WebCore {
 
 VM* g_commonVMOrNull;
-bool g_opaqueRootWriteBarrierEnabled;
 
 VM& commonVMSlow()
 {
@@ -56,18 +55,11 @@ VM& commonVMSlow()
 #endif
     
     g_commonVMOrNull->setGlobalConstRedeclarationShouldThrow(Settings::globalConstRedeclarationShouldThrow());
-    g_commonVMOrNull->heap.addMutatorShouldBeFencedCache(g_opaqueRootWriteBarrierEnabled);
     
     initNormalWorldClientData(g_commonVMOrNull);
     
     return *g_commonVMOrNull;
 }
 
-void writeBarrierOpaqueRootSlow(void* root)
-{
-    if (VM* vm = g_commonVMOrNull)
-        vm->heap.writeBarrierOpaqueRoot(root);
-}
-
 } // namespace WebCore
 
index 244ccdd..9c69675 100644 (file)
@@ -32,10 +32,8 @@ class VM;
 namespace WebCore {
 
 WEBCORE_EXPORT extern JSC::VM* g_commonVMOrNull;
-WEBCORE_EXPORT extern bool g_opaqueRootWriteBarrierEnabled;
 
 WEBCORE_EXPORT JSC::VM& commonVMSlow();
-WEBCORE_EXPORT void writeBarrierOpaqueRootSlow(void*);
 
 inline JSC::VM& commonVM()
 {
@@ -44,12 +42,5 @@ inline JSC::VM& commonVM()
     return commonVMSlow();
 }
 
-template<typename Func>
-void writeBarrierOpaqueRoot(const Func& rootThunk)
-{
-    if (g_opaqueRootWriteBarrierEnabled)
-        writeBarrierOpaqueRootSlow(rootThunk());
-}
-
 } // namespace WebCore
 
index d45cf6d..55e05c6 100644 (file)
@@ -35,6 +35,7 @@ namespace WebCore {
 
 void JSAttr::visitAdditionalChildren(JSC::SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
     if (Element* element = wrapped().ownerElement())
         visitor.addOpaqueRoot(root(element));
 }
index 6c6f3b7..95fab5a 100644 (file)
@@ -51,6 +51,7 @@ EncodedJSValue JSC_HOST_CALL jsDOMWindowInstanceFunctionShowModalDialog(ExecStat
 
 void JSDOMWindow::visitAdditionalChildren(SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
     if (Frame* frame = wrapped().frame())
         visitor.addOpaqueRoot(frame);
 }
index 4aa505f..92397cd 100644 (file)
@@ -39,6 +39,7 @@ namespace WebCore {
 
 void JSIDBCursor::visitAdditionalChildren(SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
     auto& cursor = wrapped();
     if (auto* request = cursor.request())
         visitor.addOpaqueRoot(request);
index 375dd4b..8f7e681 100644 (file)
@@ -35,6 +35,8 @@ namespace WebCore {
 
 void JSMessageChannel::visitAdditionalChildren(JSC::SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
+    
     if (MessagePort* port = wrapped().port1())
         visitor.addOpaqueRoot(port);
 
index b168cf2..5a8f37e 100644 (file)
@@ -33,6 +33,8 @@ namespace WebCore {
 
 void JSMessagePort::visitAdditionalChildren(SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
+    
     // If we have a locally entangled port, we can directly mark it as reachable. Ports that are remotely entangled are marked in-use by markActiveObjectsForContext().
     if (MessagePort* port = wrapped().locallyEntangledPort())
         visitor.addOpaqueRoot(port);
index b3f0361..25dfe8c 100644 (file)
@@ -27,6 +27,8 @@ namespace WebCore {
 
 void JSNodeIterator::visitAdditionalChildren(JSC::SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
+    
     if (NodeFilter* filter = wrapped().filter())
         visitor.addOpaqueRoot(filter);
 }
index 6e7774e..f1a010a 100644 (file)
@@ -77,6 +77,8 @@ JSValue toJS(ExecState* state, JSDOMGlobalObject* globalObject, TextTrackCue& cu
 
 void JSTextTrackCue::visitAdditionalChildren(SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
+    
     if (TextTrack* textTrack = wrapped().track())
         visitor.addOpaqueRoot(root(textTrack));
 }
index 2be08a9..76e85cd 100644 (file)
@@ -27,6 +27,8 @@ namespace WebCore {
 
 void JSTreeWalker::visitAdditionalChildren(JSC::SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
+    
     if (NodeFilter* filter = wrapped().filter())
         visitor.addOpaqueRoot(filter);
 }
index 87c9ec8..382d8eb 100644 (file)
@@ -36,6 +36,8 @@ namespace WebCore {
 
 void JSWorkerGlobalScope::visitAdditionalChildren(SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
+    
     if (auto* location = wrapped().optionalLocation())
         visitor.addOpaqueRoot(location);
     if (auto* navigator = wrapped().optionalNavigator())
index 3362f44..2bd1153 100644 (file)
@@ -58,6 +58,8 @@ namespace WebCore {
 
 void JSXMLHttpRequest::visitAdditionalChildren(SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
+    
     if (XMLHttpRequestUpload* upload = wrapped().optionalUpload())
         visitor.addOpaqueRoot(upload);
 
index d4fbbef..cfd269e 100644 (file)
@@ -33,6 +33,8 @@ namespace WebCore {
 
 void JSXPathResult::visitAdditionalChildren(JSC::SlotVisitor& visitor)
 {
+    visitor.rescanAsConstraint();
+    
     auto& value = wrapped().value();
     if (value.isNodeSet()) {
         // FIXME: This looks like it might race, but I'm not sure.
index 7f3fea6..75369d6 100644 (file)
@@ -26,7 +26,6 @@
 #include "config.h"
 #include "ContainerNodeAlgorithms.h"
 
-#include "CommonVM.h"
 #include "HTMLFrameOwnerElement.h"
 #include "InspectorInstrumentation.h"
 #include "NoEventDispatchAssertion.h"
@@ -102,8 +101,6 @@ void notifyChildNodeInserted(ContainerNode& insertionPoint, Node& node, NodeVect
         notifyNodeInsertedIntoDocument(insertionPoint, node, postInsertionNotificationTargets);
     else if (is<ContainerNode>(node))
         notifyNodeInsertedIntoTree(insertionPoint, downcast<ContainerNode>(node), postInsertionNotificationTargets);
-
-    writeBarrierOpaqueRoot([&insertionPoint] () -> void* { return insertionPoint.opaqueRoot(); });
 }
 
 void notifyNodeRemovedFromDocument(ContainerNode& insertionPoint, Node& node)
@@ -155,8 +152,6 @@ void notifyNodeRemovedFromTree(ContainerNode& insertionPoint, ContainerNode& nod
 
 void notifyChildNodeRemoved(ContainerNode& insertionPoint, Node& child)
 {
-    writeBarrierOpaqueRoot([&child] () -> void* { return &child; });
-
     if (!child.inDocument()) {
         if (is<ContainerNode>(child))
             notifyNodeRemovedFromTree(insertionPoint, downcast<ContainerNode>(child));