Slow and big Heap methods should not be inline
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Oct 2016 19:34:46 +0000 (19:34 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Oct 2016 19:34:46 +0000 (19:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=163802

Reviewed by Keith Miller.

JSC often suffers from the inline cargo cult, and Heap is a prime example. This outlines a
bunch of Heap methods that are either already very big, or call out-of-line methods, or call
very big methods, or are not called often enough for inlining to matter.

This simplifies concurrent GC work because I'm so tired of recompiling the world when I touch
one of these methods.

This looks to be perf-neutral.

* heap/Heap.cpp:
(JSC::Heap::shouldCollect):
(JSC::Heap::isCurrentThreadBusy):
(JSC::Heap::reportExtraMemoryVisited):
(JSC::Heap::reportExternalMemoryVisited):
(JSC::Heap::collectIfNecessaryOrDefer):
(JSC::Heap::collectAccordingToDeferGCProbability):
(JSC::Heap::decrementDeferralDepthAndGCIfNeeded):
(JSC::Heap::registerWeakGCMap):
(JSC::Heap::unregisterWeakGCMap):
(JSC::Heap::didAllocateBlock):
(JSC::Heap::didFreeBlock):
* heap/HeapInlines.h:
(JSC::Heap::shouldCollect): Deleted.
(JSC::Heap::isCurrentThreadBusy): Deleted.
(JSC::Heap::reportExtraMemoryVisited): Deleted.
(JSC::Heap::reportExternalMemoryVisited): Deleted.
(JSC::Heap::collectIfNecessaryOrDefer): Deleted.
(JSC::Heap::collectAccordingToDeferGCProbability): Deleted.
(JSC::Heap::decrementDeferralDepthAndGCIfNeeded): Deleted.
(JSC::Heap::registerWeakGCMap): Deleted.
(JSC::Heap::unregisterWeakGCMap): Deleted.
(JSC::Heap::didAllocateBlock): Deleted.
(JSC::Heap::didFreeBlock): Deleted.

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

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

index b8b1426..429d0a3 100644 (file)
@@ -1,3 +1,44 @@
+2016-10-21  Filip Pizlo  <fpizlo@apple.com>
+
+        Slow and big Heap methods should not be inline
+        https://bugs.webkit.org/show_bug.cgi?id=163802
+
+        Reviewed by Keith Miller.
+        
+        JSC often suffers from the inline cargo cult, and Heap is a prime example. This outlines a
+        bunch of Heap methods that are either already very big, or call out-of-line methods, or call
+        very big methods, or are not called often enough for inlining to matter.
+        
+        This simplifies concurrent GC work because I'm so tired of recompiling the world when I touch
+        one of these methods.
+        
+        This looks to be perf-neutral.
+
+        * heap/Heap.cpp:
+        (JSC::Heap::shouldCollect):
+        (JSC::Heap::isCurrentThreadBusy):
+        (JSC::Heap::reportExtraMemoryVisited):
+        (JSC::Heap::reportExternalMemoryVisited):
+        (JSC::Heap::collectIfNecessaryOrDefer):
+        (JSC::Heap::collectAccordingToDeferGCProbability):
+        (JSC::Heap::decrementDeferralDepthAndGCIfNeeded):
+        (JSC::Heap::registerWeakGCMap):
+        (JSC::Heap::unregisterWeakGCMap):
+        (JSC::Heap::didAllocateBlock):
+        (JSC::Heap::didFreeBlock):
+        * heap/HeapInlines.h:
+        (JSC::Heap::shouldCollect): Deleted.
+        (JSC::Heap::isCurrentThreadBusy): Deleted.
+        (JSC::Heap::reportExtraMemoryVisited): Deleted.
+        (JSC::Heap::reportExternalMemoryVisited): Deleted.
+        (JSC::Heap::collectIfNecessaryOrDefer): Deleted.
+        (JSC::Heap::collectAccordingToDeferGCProbability): Deleted.
+        (JSC::Heap::decrementDeferralDepthAndGCIfNeeded): Deleted.
+        (JSC::Heap::registerWeakGCMap): Deleted.
+        (JSC::Heap::unregisterWeakGCMap): Deleted.
+        (JSC::Heap::didAllocateBlock): Deleted.
+        (JSC::Heap::didFreeBlock): Deleted.
+
 2016-10-21  Saam Barati  <sbarati@apple.com>
 
         SpeculativeJIT::compileTryGetById needs to pass in NeedsToSpill along both the cell speculation and untyped speculation path
index 6bcb923..3755fe7 100644 (file)
@@ -1547,4 +1547,118 @@ void Heap::writeBarrierSlowPath(const JSCell* from)
     addToRememberedSet(from);
 }
 
+bool Heap::shouldCollect()
+{
+    if (isDeferred())
+        return false;
+    if (!m_isSafeToCollect)
+        return false;
+    if (collectionScope() || mutatorState() == MutatorState::HelpingGC)
+        return false;
+    if (Options::gcMaxHeapSize())
+        return m_bytesAllocatedThisCycle > Options::gcMaxHeapSize();
+    return m_bytesAllocatedThisCycle > m_maxEdenSize;
+}
+
+bool Heap::isCurrentThreadBusy()
+{
+    return mayBeGCThread() || mutatorState() != MutatorState::Running;
+}
+
+void Heap::reportExtraMemoryVisited(CellState oldState, size_t size)
+{
+    // We don't want to double-count the extra memory that was reported in previous collections.
+    if (collectionScope() == CollectionScope::Eden && oldState == CellState::OldGrey)
+        return;
+
+    size_t* counter = &m_extraMemorySize;
+    
+    for (;;) {
+        size_t oldSize = *counter;
+        if (WTF::weakCompareAndSwap(counter, oldSize, oldSize + size))
+            return;
+    }
+}
+
+#if ENABLE(RESOURCE_USAGE)
+void Heap::reportExternalMemoryVisited(CellState oldState, size_t size)
+{
+    // We don't want to double-count the external memory that was reported in previous collections.
+    if (collectionScope() == CollectionScope::Eden && oldState == CellState::OldGrey)
+        return;
+
+    size_t* counter = &m_externalMemorySize;
+
+    for (;;) {
+        size_t oldSize = *counter;
+        if (WTF::weakCompareAndSwap(counter, oldSize, oldSize + size))
+            return;
+    }
+}
+#endif
+
+bool Heap::collectIfNecessaryOrDefer(GCDeferralContext* deferralContext)
+{
+    if (!shouldCollect())
+        return false;
+
+    if (deferralContext)
+        deferralContext->m_shouldGC = true;
+    else
+        collect();
+    return true;
+}
+
+void Heap::collectAccordingToDeferGCProbability()
+{
+    if (isDeferred() || !m_isSafeToCollect || collectionScope() || mutatorState() == MutatorState::HelpingGC)
+        return;
+
+    if (randomNumber() < Options::deferGCProbability()) {
+        collect();
+        return;
+    }
+
+    // If our coin flip told us not to GC, we still might GC,
+    // but we GC according to our memory pressure markers.
+    collectIfNecessaryOrDefer();
+}
+
+void Heap::decrementDeferralDepthAndGCIfNeeded()
+{
+    decrementDeferralDepth();
+    if (UNLIKELY(Options::deferGCShouldCollectWithProbability()))
+        collectAccordingToDeferGCProbability();
+    else
+        collectIfNecessaryOrDefer();
+}
+
+void Heap::registerWeakGCMap(void* weakGCMap, std::function<void()> pruningCallback)
+{
+    m_weakGCMaps.add(weakGCMap, WTFMove(pruningCallback));
+}
+
+void Heap::unregisterWeakGCMap(void* weakGCMap)
+{
+    m_weakGCMaps.remove(weakGCMap);
+}
+
+void Heap::didAllocateBlock(size_t capacity)
+{
+#if ENABLE(RESOURCE_USAGE)
+    m_blockBytesAllocated += capacity;
+#else
+    UNUSED_PARAM(capacity);
+#endif
+}
+
+void Heap::didFreeBlock(size_t capacity)
+{
+#if ENABLE(RESOURCE_USAGE)
+    m_blockBytesAllocated -= capacity;
+#else
+    UNUSED_PARAM(capacity);
+#endif
+}
+
 } // namespace JSC
index 0a490de..650472c 100644 (file)
@@ -145,7 +145,7 @@ public:
 
     // We're always busy on the collection threads. On the main thread, this returns true if we're
     // helping heap.
-    bool isCurrentThreadBusy();
+    JS_EXPORT_PRIVATE bool isCurrentThreadBusy();
     
     MarkedSpace::Subspace& subspaceForObjectWithoutDestructor() { return m_objectSpace.subspaceForObjectsWithoutDestructor(); }
     MarkedSpace::Subspace& subspaceForObjectDestructor() { return m_objectSpace.subspaceForObjectsWithDestructor(); }
@@ -184,11 +184,11 @@ public:
     // call both of these functions: Calling only one may trigger catastropic
     // memory growth.
     void reportExtraMemoryAllocated(size_t);
-    void reportExtraMemoryVisited(CellState oldState, size_t);
+    JS_EXPORT_PRIVATE void reportExtraMemoryVisited(CellState oldState, size_t);
 
 #if ENABLE(RESOURCE_USAGE)
     // Use this API to report the subset of extra memory that lives outside this process.
-    void reportExternalMemoryVisited(CellState oldState, size_t);
+    JS_EXPORT_PRIVATE void reportExternalMemoryVisited(CellState oldState, size_t);
     size_t externalMemorySize() { return m_externalMemorySize; }
 #endif
 
@@ -252,8 +252,8 @@ public:
 
     static bool isZombified(JSCell* cell) { return *(void**)cell == zombifiedBits; }
 
-    void registerWeakGCMap(void* weakGCMap, std::function<void()> pruningCallback);
-    void unregisterWeakGCMap(void* weakGCMap);
+    JS_EXPORT_PRIVATE void registerWeakGCMap(void* weakGCMap, std::function<void()> pruningCallback);
+    JS_EXPORT_PRIVATE void unregisterWeakGCMap(void* weakGCMap);
 
     void addLogicallyEmptyWeakBlock(WeakBlock*);
 
@@ -374,7 +374,7 @@ private:
 
     void incrementDeferralDepth();
     void decrementDeferralDepth();
-    void decrementDeferralDepthAndGCIfNeeded();
+    JS_EXPORT_PRIVATE void decrementDeferralDepthAndGCIfNeeded();
 
     size_t threadVisitCount();
     size_t threadBytesVisited();
index e9a8e92..16aa1ef 100644 (file)
 
 namespace JSC {
 
-inline bool Heap::shouldCollect()
-{
-    if (isDeferred())
-        return false;
-    if (!m_isSafeToCollect)
-        return false;
-    if (collectionScope() || mutatorState() == MutatorState::HelpingGC)
-        return false;
-    if (Options::gcMaxHeapSize())
-        return m_bytesAllocatedThisCycle > Options::gcMaxHeapSize();
-    return m_bytesAllocatedThisCycle > m_maxEdenSize;
-}
-
-inline bool Heap::isCurrentThreadBusy()
-{
-    return mayBeGCThread() || mutatorState() != MutatorState::Running;
-}
-
 ALWAYS_INLINE Heap* Heap::heap(const HeapCell* cell)
 {
     return cell->heap();
@@ -147,50 +129,6 @@ inline void Heap::writeBarrierWithoutFence(const JSCell* from)
         addToRememberedSet(from);
 }
 
-inline void Heap::reportExtraMemoryAllocated(size_t size)
-{
-    if (size > minExtraMemory) 
-        reportExtraMemoryAllocatedSlowCase(size);
-}
-
-inline void Heap::reportExtraMemoryVisited(CellState oldState, size_t size)
-{
-    // We don't want to double-count the extra memory that was reported in previous collections.
-    if (collectionScope() == CollectionScope::Eden && oldState == CellState::OldGrey)
-        return;
-
-    size_t* counter = &m_extraMemorySize;
-    
-    for (;;) {
-        size_t oldSize = *counter;
-        if (WTF::weakCompareAndSwap(counter, oldSize, oldSize + size))
-            return;
-    }
-}
-
-#if ENABLE(RESOURCE_USAGE)
-inline void Heap::reportExternalMemoryVisited(CellState oldState, size_t size)
-{
-    // We don't want to double-count the external memory that was reported in previous collections.
-    if (collectionScope() == CollectionScope::Eden && oldState == CellState::OldGrey)
-        return;
-
-    size_t* counter = &m_externalMemorySize;
-
-    for (;;) {
-        size_t oldSize = *counter;
-        if (WTF::weakCompareAndSwap(counter, oldSize, oldSize + size))
-            return;
-    }
-}
-#endif
-
-inline void Heap::deprecatedReportExtraMemory(size_t size)
-{
-    if (size > minExtraMemory) 
-        deprecatedReportExtraMemorySlowCase(size);
-}
-
 template<typename Functor> inline void Heap::forEachCodeBlock(const Functor& func)
 {
     forEachCodeBlockImpl(scopedLambdaRef<bool(CodeBlock*)>(func));
@@ -353,42 +291,6 @@ inline void Heap::decrementDeferralDepth()
     m_deferralDepth--;
 }
 
-inline bool Heap::collectIfNecessaryOrDefer(GCDeferralContext* deferralContext)
-{
-    if (!shouldCollect())
-        return false;
-
-    if (deferralContext)
-        deferralContext->m_shouldGC = true;
-    else
-        collect();
-    return true;
-}
-
-inline void Heap::collectAccordingToDeferGCProbability()
-{
-    if (isDeferred() || !m_isSafeToCollect || collectionScope() || mutatorState() == MutatorState::HelpingGC)
-        return;
-
-    if (randomNumber() < Options::deferGCProbability()) {
-        collect();
-        return;
-    }
-
-    // If our coin flip told us not to GC, we still might GC,
-    // but we GC according to our memory pressure markers.
-    collectIfNecessaryOrDefer();
-}
-
-inline void Heap::decrementDeferralDepthAndGCIfNeeded()
-{
-    decrementDeferralDepth();
-    if (UNLIKELY(Options::deferGCShouldCollectWithProbability()))
-        collectAccordingToDeferGCProbability();
-    else
-        collectIfNecessaryOrDefer();
-}
-
 inline HashSet<MarkedArgumentBuffer*>& Heap::markListSet()
 {
     if (!m_markListSet)
@@ -396,32 +298,16 @@ inline HashSet<MarkedArgumentBuffer*>& Heap::markListSet()
     return *m_markListSet;
 }
 
-inline void Heap::registerWeakGCMap(void* weakGCMap, std::function<void()> pruningCallback)
-{
-    m_weakGCMaps.add(weakGCMap, WTFMove(pruningCallback));
-}
-
-inline void Heap::unregisterWeakGCMap(void* weakGCMap)
-{
-    m_weakGCMaps.remove(weakGCMap);
-}
-
-inline void Heap::didAllocateBlock(size_t capacity)
+inline void Heap::reportExtraMemoryAllocated(size_t size)
 {
-#if ENABLE(RESOURCE_USAGE)
-    m_blockBytesAllocated += capacity;
-#else
-    UNUSED_PARAM(capacity);
-#endif
+    if (size > minExtraMemory) 
+        reportExtraMemoryAllocatedSlowCase(size);
 }
 
-inline void Heap::didFreeBlock(size_t capacity)
+inline void Heap::deprecatedReportExtraMemory(size_t size)
 {
-#if ENABLE(RESOURCE_USAGE)
-    m_blockBytesAllocated -= capacity;
-#else
-    UNUSED_PARAM(capacity);
-#endif
+    if (size > minExtraMemory) 
+        deprecatedReportExtraMemorySlowCase(size);
 }
 
 } // namespace JSC