[JSC] Revert r226885 to make SlotVisitor creation lazy
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Feb 2019 05:37:52 +0000 (05:37 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Feb 2019 05:37:52 +0000 (05:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195013

Reviewed by Saam Barati.

We once changed SlotVisitor creation apriori to drop the lock. Also, it turns out that SlotVisitor is memory-consuming.
We should defer SlotVisitor creation until it is actually required. This patch reverts r226885. Even with this patch,
we still hold many SlotVisitors after we execute many parallel markers at least once. But recovering the feature of
dynamically allocating SlotVisitors helps further memory optimizations in this area.

* heap/Heap.cpp:
(JSC::Heap::Heap):
(JSC::Heap::runBeginPhase):
* heap/Heap.h:
* heap/HeapInlines.h:
(JSC::Heap::forEachSlotVisitor):
(JSC::Heap::numberOfSlotVisitors):
* heap/MarkingConstraintSolver.cpp:
(JSC::MarkingConstraintSolver::didVisitSomething const):
* heap/SlotVisitor.h:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/HeapInlines.h
Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp
Source/JavaScriptCore/heap/SlotVisitor.h

index 81d2b8b..fe4d36b 100644 (file)
@@ -1,3 +1,26 @@
+2019-02-25  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] Revert r226885 to make SlotVisitor creation lazy
+        https://bugs.webkit.org/show_bug.cgi?id=195013
+
+        Reviewed by Saam Barati.
+
+        We once changed SlotVisitor creation apriori to drop the lock. Also, it turns out that SlotVisitor is memory-consuming.
+        We should defer SlotVisitor creation until it is actually required. This patch reverts r226885. Even with this patch,
+        we still hold many SlotVisitors after we execute many parallel markers at least once. But recovering the feature of
+        dynamically allocating SlotVisitors helps further memory optimizations in this area.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::Heap):
+        (JSC::Heap::runBeginPhase):
+        * heap/Heap.h:
+        * heap/HeapInlines.h:
+        (JSC::Heap::forEachSlotVisitor):
+        (JSC::Heap::numberOfSlotVisitors):
+        * heap/MarkingConstraintSolver.cpp:
+        (JSC::MarkingConstraintSolver::didVisitSomething const):
+        * heap/SlotVisitor.h:
+
 2019-02-25  Saam Barati  <sbarati@apple.com>
 
         testb3 and testair should test O0/O1/O2
index 2ad0e0a..094a785 100644 (file)
@@ -300,14 +300,6 @@ Heap::Heap(VM* vm, HeapType heapType)
     , m_threadCondition(AutomaticThreadCondition::create())
 {
     m_worldState.store(0);
-
-    for (unsigned i = 0, numberOfParallelThreads = heapHelperPool().numberOfThreads(); i < numberOfParallelThreads; ++i) {
-        std::unique_ptr<SlotVisitor> visitor = std::make_unique<SlotVisitor>(*this, toCString("P", i + 1));
-        if (Options::optimizeParallelSlotVisitorsForStoppedMutator())
-            visitor->optimizeForStoppedMutator();
-        m_availableParallelSlotVisitors.append(visitor.get());
-        m_parallelSlotVisitors.append(WTFMove(visitor));
-    }
     
     if (Options::useConcurrentGC()) {
         if (Options::useStochasticMutatorScheduler())
@@ -1261,8 +1253,19 @@ NEVER_INLINE bool Heap::runBeginPhase(GCConductor conn)
             SlotVisitor* slotVisitor;
             {
                 LockHolder locker(m_parallelSlotVisitorLock);
-                RELEASE_ASSERT_WITH_MESSAGE(!m_availableParallelSlotVisitors.isEmpty(), "Parallel SlotVisitors are allocated apriori");
-                slotVisitor = m_availableParallelSlotVisitors.takeLast();
+                if (m_availableParallelSlotVisitors.isEmpty()) {
+                    std::unique_ptr<SlotVisitor> newVisitor = std::make_unique<SlotVisitor>(
+                        *this, toCString("P", m_parallelSlotVisitors.size() + 1));
+                    
+                    if (Options::optimizeParallelSlotVisitorsForStoppedMutator())
+                        newVisitor->optimizeForStoppedMutator();
+                    
+                    newVisitor->didStartMarking();
+                    
+                    slotVisitor = newVisitor.get();
+                    m_parallelSlotVisitors.append(WTFMove(newVisitor));
+                } else
+                    slotVisitor = m_availableParallelSlotVisitors.takeLast();
             }
 
             WTF::registerGCThread(GCThreadType::Helper);
index 248881f..5912f99 100644 (file)
@@ -393,6 +393,7 @@ public:
 
     template<typename Func>
     void forEachSlotVisitor(const Func&);
+    unsigned numberOfSlotVisitors();
     
     Seconds totalGCTime() const { return m_totalGCTime; }
 
index defa4ec..f5b0d49 100644 (file)
@@ -275,10 +275,17 @@ inline void Heap::stopIfNecessary()
 template<typename Func>
 void Heap::forEachSlotVisitor(const Func& func)
 {
+    auto locker = holdLock(m_parallelSlotVisitorLock);
     func(*m_collectorSlotVisitor);
     func(*m_mutatorSlotVisitor);
     for (auto& slotVisitor : m_parallelSlotVisitors)
         func(*slotVisitor);
 }
 
+inline unsigned Heap::numberOfSlotVisitors()
+{
+    auto locker = holdLock(m_parallelSlotVisitorLock);
+    return m_parallelSlotVisitors.size() + 2; // m_collectorSlotVisitor and m_mutatorSlotVisitor
+}
+
 } // namespace JSC
index a7ff046..4cff499 100644 (file)
@@ -51,6 +51,10 @@ bool MarkingConstraintSolver::didVisitSomething() const
         if (visitCounter.visitCount())
             return true;
     }
+    // If the number of SlotVisitors increases after creating m_visitCounters,
+    // we conservatively say there could be something visited by added SlotVisitors.
+    if (m_heap.numberOfSlotVisitors() > m_visitCounters.size())
+        return true;
     return false;
 }
 
index 77118d1..782fa44 100644 (file)
@@ -259,8 +259,6 @@ private:
     MarkingConstraint* m_currentConstraint { nullptr };
     MarkingConstraintSolver* m_currentSolver { nullptr };
     
-    // Put padding here to mitigate false sharing between multiple SlotVisitors.
-    char padding[64];
 public:
 #if !ASSERT_DISABLED
     bool m_isCheckingForDefaultMarkViolation;