Changeset 242070 in webkit

Timestamp:

Feb 25, 2019, 9:37:52 PM (8 years ago)

Author:

ysuzuki@apple.com

Message:

[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:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• heap/Heap.cpp (modified) (2 diffs)
• heap/Heap.h (modified) (1 diff)
• heap/HeapInlines.h (modified) (2 diffs)
• heap/MarkingConstraintSolver.cpp (modified) (1 diff)
• heap/SlotVisitor.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/heap/Heap.cpp

@@ -301 +301 @@
 {
     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()) {
@@ -1262 +1254 @@
             {
                 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();
             }
 

trunk/Source/JavaScriptCore/heap/Heap.h

@@ -394 +394 @@
     template<typename Func>
     void forEachSlotVisitor(const Func&);
+    unsigned numberOfSlotVisitors();
     
     Seconds totalGCTime() const { return m_totalGCTime; }

trunk/Source/JavaScriptCore/heap/HeapInlines.h

@@ -276 +276 @@
 void Heap::forEachSlotVisitor(const Func& func)
 {
+    auto locker = holdLock(m_parallelSlotVisitorLock);
     func(*m_collectorSlotVisitor);
     func(*m_mutatorSlotVisitor);
@@ -282 +283 @@
 }
 
+inline unsigned Heap::numberOfSlotVisitors()
+{
+    auto locker = holdLock(m_parallelSlotVisitorLock);
+    return m_parallelSlotVisitors.size() + 2; // m_collectorSlotVisitor and m_mutatorSlotVisitor
+}
+
 } // namespace JSC

trunk/Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp

@@ -52 +52 @@
             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;
 }

trunk/Source/JavaScriptCore/heap/SlotVisitor.h

@@ -260 +260 @@
     MarkingConstraintSolver* m_currentSolver { nullptr };
     
-    // Put padding here to mitigate false sharing between multiple SlotVisitors.
-    char padding[64];
 public:
 #if !ASSERT_DISABLED