Allow one sync GC per gcTimer interval on critical memory pressure warning
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jun 2015 22:15:36 +0000 (22:15 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 9 Jun 2015 22:15:36 +0000 (22:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=145773

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

On critical memory pressure warning, we were calling GCController::garbageCollectSoon(),
which does not offer any guarantee on when the garbage collection will actually take
place.

On critical memory pressure, we need to free up memory as soon as possible to avoid
getting killed so this is an issue. Also, the fact that we clear the PageCache on
critical memory pressure means a GC would likely be useful, even if the last
collection did not free much memory.

This patch adds a new GCController::garbageCollectNowIfNotDoneRecently() API that allows
one synchronous GC per gcTimer interval on critical memory pressure warning. This makes
us more responsive to critical memory pressure and avoids doing synchronous GCs too
often.

* heap/FullGCActivityCallback.cpp:
(JSC::FullGCActivityCallback::doCollection):
* heap/FullGCActivityCallback.h:
(JSC::GCActivityCallback::createFullTimer):
* heap/GCActivityCallback.h:
* heap/Heap.cpp:
(JSC::Heap::collectAllGarbageIfNotDoneRecently):
* heap/Heap.h:

* heap/IncrementalSweeper.cpp:
(JSC::IncrementalSweeper::doWork): Deleted.
* heap/IncrementalSweeper.h:

Drop fullSweep() API as it no longer seems useful. garbageCollectNow()
already does a sweep after the full collection.

Source/WebCore:

* bindings/js/GCController.cpp:
(WebCore::GCController::garbageCollectNowIfNotDoneRecently):

Add new GCController::garbageCollectNowIfNotDoneRecently() API that
allows one synchronous GC per full GC timer interval. If called more
than once per interval, it becomes equivalent to garbageCollectSoon()
and merely accelerates the next collection.

* bindings/js/GCController.h:
* platform/MemoryPressureHandler.cpp:
(WebCore::MemoryPressureHandler::releaseCriticalMemory):

Call the new GCController::garbageCollectNowIfNotDoneRecently() on
critical memory pressure instead of garbageCollectionSoon() to try
as do a synchronous GC if one wasn't already done recently.
Also drop call to fullSweep() as GCController::garbageCollectNow*()
already do a sweep after the collection.

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

12 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/FullGCActivityCallback.cpp
Source/JavaScriptCore/heap/FullGCActivityCallback.h
Source/JavaScriptCore/heap/GCActivityCallback.h
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/IncrementalSweeper.cpp
Source/JavaScriptCore/heap/IncrementalSweeper.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/GCController.cpp
Source/WebCore/bindings/js/GCController.h
Source/WebCore/platform/MemoryPressureHandler.cpp

index 3085e93..138d530 100644 (file)
@@ -1,3 +1,40 @@
+2015-06-09  Chris Dumez  <cdumez@apple.com>
+
+        Allow one sync GC per gcTimer interval on critical memory pressure warning
+        https://bugs.webkit.org/show_bug.cgi?id=145773
+
+        Reviewed by Geoffrey Garen.
+
+        On critical memory pressure warning, we were calling GCController::garbageCollectSoon(),
+        which does not offer any guarantee on when the garbage collection will actually take
+        place.
+
+        On critical memory pressure, we need to free up memory as soon as possible to avoid
+        getting killed so this is an issue. Also, the fact that we clear the PageCache on
+        critical memory pressure means a GC would likely be useful, even if the last
+        collection did not free much memory.
+
+        This patch adds a new GCController::garbageCollectNowIfNotDoneRecently() API that allows
+        one synchronous GC per gcTimer interval on critical memory pressure warning. This makes
+        us more responsive to critical memory pressure and avoids doing synchronous GCs too
+        often.
+
+        * heap/FullGCActivityCallback.cpp:
+        (JSC::FullGCActivityCallback::doCollection):
+        * heap/FullGCActivityCallback.h:
+        (JSC::GCActivityCallback::createFullTimer):
+        * heap/GCActivityCallback.h:
+        * heap/Heap.cpp:
+        (JSC::Heap::collectAllGarbageIfNotDoneRecently):
+        * heap/Heap.h:
+
+        * heap/IncrementalSweeper.cpp:
+        (JSC::IncrementalSweeper::doWork): Deleted.
+        * heap/IncrementalSweeper.h:
+
+        Drop fullSweep() API as it no longer seems useful. garbageCollectNow()
+        already does a sweep after the full collection.
+
 2015-06-09  Andreas Kling  <akling@apple.com>
 
         [JSC] CodeBlock::m_constantRegisters should be sized-to-fit.
index 76c678d..07ebbbd 100644 (file)
@@ -43,18 +43,19 @@ FullGCActivityCallback::FullGCActivityCallback(Heap* heap)
 
 void FullGCActivityCallback::doCollection()
 {
-    Heap* heap = &m_vm->heap;
+    Heap& heap = m_vm->heap;
+    m_didSyncGCRecently = false;
 
 #if !PLATFORM(IOS)
     double startTime = WTF::monotonicallyIncreasingTime();
-    if (heap->isPagedOut(startTime + pagingTimeOut)) {
+    if (heap.isPagedOut(startTime + pagingTimeOut)) {
         cancel();
-        heap->increaseLastFullGCLength(pagingTimeOut);
+        heap.increaseLastFullGCLength(pagingTimeOut);
         return;
     }
 #endif
 
-    heap->collect(FullCollection);
+    heap.collect(FullCollection);
 }
 
 double FullGCActivityCallback::lastGCLength()
index 3b1b450..e727592 100644 (file)
@@ -36,6 +36,9 @@ public:
 
     virtual void doCollection() override;
 
+    bool didSyncGCRecently() const { return m_didSyncGCRecently; }
+    void setDidSyncGCRecently() { m_didSyncGCRecently = true; }
+
 protected:
 #if USE(CF)
     FullGCActivityCallback(Heap* heap, CFRunLoopRef runLoop)
@@ -47,9 +50,11 @@ protected:
     virtual double lastGCLength() override;
     virtual double gcTimeSlice(size_t bytes) override;
     virtual double deathRate() override;
+
+    bool m_didSyncGCRecently { false };
 };
 
-inline RefPtr<GCActivityCallback> GCActivityCallback::createFullTimer(Heap* heap)
+inline RefPtr<FullGCActivityCallback> GCActivityCallback::createFullTimer(Heap* heap)
 {
     return s_shouldCreateGCTimer ? adoptRef(new FullGCActivityCallback(heap)) : nullptr;
 }
index 3d07efa..73d0e89 100644 (file)
 
 namespace JSC {
 
+class FullGCActivityCallback;
 class Heap;
 
 class JS_EXPORT_PRIVATE GCActivityCallback : public HeapTimer, public ThreadSafeRefCounted<GCActivityCallback> {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    static RefPtr<GCActivityCallback> createFullTimer(Heap*);
+    static RefPtr<FullGCActivityCallback> createFullTimer(Heap*);
     static RefPtr<GCActivityCallback> createEdenTimer(Heap*);
 
     GCActivityCallback(Heap*);
index d52f537..5283602 100644 (file)
@@ -1408,6 +1408,23 @@ void Heap::addCompiledCode(ExecutableBase* executable)
     m_compiledCode.append(executable);
 }
 
+void Heap::collectAllGarbageIfNotDoneRecently()
+{
+    if (!m_fullActivityCallback) {
+        collectAllGarbage();
+        return;
+    }
+
+    if (m_fullActivityCallback->didSyncGCRecently()) {
+        // A synchronous GC was already requested recently so we merely accelerate next collection.
+        reportAbandonedObjectGraph();
+        return;
+    }
+
+    m_fullActivityCallback->setDidSyncGCRecently();
+    collectAllGarbage();
+}
+
 class Zombify : public MarkedBlock::VoidFunctor {
 public:
     inline void visit(JSCell* cell)
index d3e9688..878fb5c 100644 (file)
@@ -154,6 +154,7 @@ public:
     void notifyIsSafeToCollect() { m_isSafeToCollect = true; }
     bool isSafeToCollect() const { return m_isSafeToCollect; }
 
+    JS_EXPORT_PRIVATE void collectAllGarbageIfNotDoneRecently();
     void collectAllGarbage() { collectAndSweep(FullCollection); }
     JS_EXPORT_PRIVATE void collectAndSweep(HeapOperation collectionType = AnyCollection);
     bool shouldCollect();
@@ -394,7 +395,7 @@ private:
     Vector<WeakBlock*> m_logicallyEmptyWeakBlocks;
     size_t m_indexOfNextLogicallyEmptyWeakBlockToSweep { WTF::notFound };
     
-    RefPtr<GCActivityCallback> m_fullActivityCallback;
+    RefPtr<FullGCActivityCallback> m_fullActivityCallback;
     RefPtr<GCActivityCallback> m_edenActivityCallback;
     std::unique_ptr<IncrementalSweeper> m_sweeper;
     Vector<MarkedBlock*> m_blockSnapshot;
index 1e1957e..e0783d6 100644 (file)
@@ -59,11 +59,6 @@ void IncrementalSweeper::cancelTimer()
     CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + s_decade);
 }
 
-void IncrementalSweeper::fullSweep()
-{
-    while (sweepNextBlock()) { }
-}
-
 void IncrementalSweeper::doWork()
 {
     doSweep(WTF::monotonicallyIncreasingTime());
@@ -132,10 +127,6 @@ void IncrementalSweeper::willFinishSweeping()
 {
 }
 
-void IncrementalSweeper::fullSweep()
-{
-}
-
 bool IncrementalSweeper::sweepNextBlock()
 {
     return false;
index 59620a0..a86fd1c 100644 (file)
@@ -43,7 +43,6 @@ public:
     explicit IncrementalSweeper(VM*);
 #endif
 
-    JS_EXPORT_PRIVATE void fullSweep();
     void startSweeping();
 
     JS_EXPORT_PRIVATE virtual void doWork() override;
index 69f1d58..b3c987d 100644 (file)
@@ -1,3 +1,28 @@
+2015-06-09  Chris Dumez  <cdumez@apple.com>
+
+        Allow one sync GC per gcTimer interval on critical memory pressure warning
+        https://bugs.webkit.org/show_bug.cgi?id=145773
+
+        Reviewed by Geoffrey Garen.
+
+        * bindings/js/GCController.cpp:
+        (WebCore::GCController::garbageCollectNowIfNotDoneRecently):
+
+        Add new GCController::garbageCollectNowIfNotDoneRecently() API that
+        allows one synchronous GC per full GC timer interval. If called more
+        than once per interval, it becomes equivalent to garbageCollectSoon()
+        and merely accelerates the next collection.
+
+        * bindings/js/GCController.h:
+        * platform/MemoryPressureHandler.cpp:
+        (WebCore::MemoryPressureHandler::releaseCriticalMemory):
+
+        Call the new GCController::garbageCollectNowIfNotDoneRecently() on
+        critical memory pressure instead of garbageCollectionSoon() to try
+        as do a synchronous GC if one wasn't already done recently.
+        Also drop call to fullSweep() as GCController::garbageCollectNow*()
+        already do a sweep after the collection.
+
 2015-06-09  Darin Adler  <darin@apple.com>
 
         Follow-up fix for:
index c0a2bc4..e58f69a 100644 (file)
@@ -90,6 +90,17 @@ void GCController::garbageCollectNow()
     }
 }
 
+void GCController::garbageCollectNowIfNotDoneRecently()
+{
+#if USE(CF)
+    JSLockHolder lock(JSDOMWindow::commonVM());
+    if (!JSDOMWindow::commonVM().heap.isBusy())
+        JSDOMWindow::commonVM().heap.collectAllGarbageIfNotDoneRecently();
+#else
+    garbageCollectSoon();
+#endif
+}
+
 void GCController::garbageCollectOnAlternateThreadForDebugging(bool waitUntilDone)
 {
     ThreadIdentifier threadID = createThread(collect, 0, "WebCore: GCController");
index fa31cda..9720c3c 100644 (file)
@@ -40,6 +40,7 @@ public:
 
     WEBCORE_EXPORT void garbageCollectSoon();
     WEBCORE_EXPORT void garbageCollectNow(); // It's better to call garbageCollectSoon, unless you have a specific reason not to.
+    WEBCORE_EXPORT void garbageCollectNowIfNotDoneRecently();
     void garbageCollectOnNextRunLoop();
 
     WEBCORE_EXPORT void garbageCollectOnAlternateThreadForDebugging(bool waitUntilDone); // Used for stress testing.
index 9bafd74..154b546 100644 (file)
@@ -139,19 +139,8 @@ void MemoryPressureHandler::releaseCriticalMemory(Synchronous synchronous)
     if (synchronous == Synchronous::Yes) {
         ReliefLogger log("Collecting JavaScript garbage");
         GCController::singleton().garbageCollectNow();
-    } else {
-        // FIXME: We should do a garbage sweep and prune dead resources from the MemoryCache
-        // after the garbage collection has completed to free up more memory.
-        GCController::singleton().garbageCollectSoon();
-
-        // Do a full sweep of collected objects. garbageCollectNow() already does this so we only
-        // need to do this if it isn't called.
-        {
-            ReliefLogger log("Full JavaScript garbage sweep");
-            JSC::JSLockHolder lock(JSDOMWindow::commonVM());
-            JSDOMWindow::commonVM().heap.sweeper()->fullSweep();
-        }
-    }
+    } else
+        GCController::singleton().garbageCollectNowIfNotDoneRecently();
 }
 
 void MemoryPressureHandler::releaseMemory(Critical critical, Synchronous synchronous)