Title: [115288] trunk/Source
Revision
115288
Author
mhahnenb...@apple.com
Date
2012-04-25 22:08:06 -0700 (Wed, 25 Apr 2012)

Log Message

WebCore shouldn't call collectAllGarbage directly
https://bugs.webkit.org/show_bug.cgi?id=84897

Reviewed by Geoffrey Garen.

Source/_javascript_Core:

* _javascript_Core.vcproj/_javascript_Core/_javascript_Core.def: Exported symbol
for reportAbanondedObjectGraph so WebCore can use it.
* heap/Heap.h: Ditto.

Source/WebCore:

No new tests.

Currently, GCController calls Heap::collectAllGarbage directly, which leads
to an overload of collections as the timer in GCController and the timer in
GCActivityCallback compete for collection time and fire independently. As a
result, we end up doing almost 600 full collections during an in-browser run
of SunSpider, or 20 full collections on a single load of TechCrunch.

We can do better by preventing WebCore from calling collectAllGarbage directly
and instead going through Heap::reportAbandonedObjectGraph, since that is what
WebCore is trying to do--notify the Heap that a lot of garbage may have just
been generated when we left a page.

* WebCore.exp.in:
* bindings/js/GCController.cpp: Removed all timer stuff.
(WebCore::GCController::GCController):
(WebCore::GCController::garbageCollectSoon): Changed to call Heap::reportAbandonedObjectGraph.
(WebCore::GCController::garbageCollectNow): Changed to still directly call collectAllGarbage.
We will deprecate this function soon hopefully.
* bindings/js/GCController.h: Removed timer stuff.
(GCController):
* bindings/js/ScriptProfiler.cpp:
(WebCore::ScriptProfiler::collectGarbage): Changed to call garbageCollectSoon.

Source/WebKit2:

* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::didClose): Changed to call garbageCollectSoon. This is the
function that causes us to do so much collection on page navigation.

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (115287 => 115288)


--- trunk/Source/_javascript_Core/ChangeLog	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/_javascript_Core/ChangeLog	2012-04-26 05:08:06 UTC (rev 115288)
@@ -1,3 +1,14 @@
+2012-04-25  Mark Hahnenberg  <mhahnenb...@apple.com>
+
+        WebCore shouldn't call collectAllGarbage directly
+        https://bugs.webkit.org/show_bug.cgi?id=84897
+
+        Reviewed by Geoffrey Garen.
+
+        * _javascript_Core.vcproj/_javascript_Core/_javascript_Core.def: Exported symbol 
+        for reportAbanondedObjectGraph so WebCore can use it.
+        * heap/Heap.h: Ditto.
+
 2012-04-25  Oliver Hunt  <oli...@apple.com>
 
         Biolab disaster crashes on ToT

Modified: trunk/Source/_javascript_Core/_javascript_Core.vcproj/_javascript_Core/_javascript_Core.def (115287 => 115288)


--- trunk/Source/_javascript_Core/_javascript_Core.vcproj/_javascript_Core/_javascript_Core.def	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/_javascript_Core/_javascript_Core.vcproj/_javascript_Core/_javascript_Core.def	2012-04-26 05:08:06 UTC (rev 115288)
@@ -280,6 +280,7 @@
     ?releaseDecommitted@OSAllocator@WTF@@SAXPAXI@Z
     ?releaseExecutableMemory@JSGlobalData@JSC@@QAEXXZ
     ?removeBlock@MarkedAllocator@JSC@@QAEXPAVMarkedBlock@2@@Z
+    ?reportAbandonedObjectGraph@Heap@JSC@@QAEXXZ
     ?reportExtraMemoryCostSlowCase@Heap@JSC@@AAEXI@Z
     ?reserveAndCommit@OSAllocator@WTF@@SAPAXIW4Usage@12@_N11@Z
     ?reserveCapacity@StringBuilder@WTF@@QAEXI@Z

Modified: trunk/Source/_javascript_Core/heap/Heap.h (115287 => 115288)


--- trunk/Source/_javascript_Core/heap/Heap.h	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/_javascript_Core/heap/Heap.h	2012-04-26 05:08:06 UTC (rev 115288)
@@ -119,7 +119,7 @@
         void collect(SweepToggle);
 
         void reportExtraMemoryCost(size_t cost);
-        void reportAbandonedObjectGraph();
+        JS_EXPORT_PRIVATE void reportAbandonedObjectGraph();
 
         JS_EXPORT_PRIVATE void protect(JSValue);
         JS_EXPORT_PRIVATE bool unprotect(JSValue); // True when the protect count drops to 0.

Modified: trunk/Source/WebCore/ChangeLog (115287 => 115288)


--- trunk/Source/WebCore/ChangeLog	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/WebCore/ChangeLog	2012-04-26 05:08:06 UTC (rev 115288)
@@ -1,3 +1,34 @@
+2012-04-25  Mark Hahnenberg  <mhahnenb...@apple.com>
+
+        WebCore shouldn't call collectAllGarbage directly
+        https://bugs.webkit.org/show_bug.cgi?id=84897
+
+        Reviewed by Geoffrey Garen.
+
+        No new tests. 
+
+        Currently, GCController calls Heap::collectAllGarbage directly, which leads 
+        to an overload of collections as the timer in GCController and the timer in 
+        GCActivityCallback compete for collection time and fire independently. As a 
+        result, we end up doing almost 600 full collections during an in-browser run 
+        of SunSpider, or 20 full collections on a single load of TechCrunch. 
+
+        We can do better by preventing WebCore from calling collectAllGarbage directly 
+        and instead going through Heap::reportAbandonedObjectGraph, since that is what 
+        WebCore is trying to do--notify the Heap that a lot of garbage may have just 
+        been generated when we left a page.
+
+        * WebCore.exp.in:
+        * bindings/js/GCController.cpp: Removed all timer stuff.
+        (WebCore::GCController::GCController):
+        (WebCore::GCController::garbageCollectSoon): Changed to call Heap::reportAbandonedObjectGraph.
+        (WebCore::GCController::garbageCollectNow): Changed to still directly call collectAllGarbage.
+        We will deprecate this function soon hopefully.
+        * bindings/js/GCController.h: Removed timer stuff.
+        (GCController):
+        * bindings/js/ScriptProfiler.cpp:
+        (WebCore::ScriptProfiler::collectGarbage): Changed to call garbageCollectSoon.
+
 2012-04-25  James Robinson  <jam...@chromium.org>
 
         [chromium] REGRESSION(112286) Compositor initialization blocks for program compilation / linking

Modified: trunk/Source/WebCore/WebCore.exp.in (115287 => 115288)


--- trunk/Source/WebCore/WebCore.exp.in	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/WebCore/WebCore.exp.in	2012-04-26 05:08:06 UTC (rev 115288)
@@ -250,6 +250,7 @@
 __ZN7WebCore12EventHandler8keyEventERKNS_21PlatformKeyboardEventE
 __ZN7WebCore12EventHandler9mouseDownEP7NSEvent
 __ZN7WebCore12GCController17garbageCollectNowEv
+__ZN7WebCore12GCController18garbageCollectSoonEv
 __ZN7WebCore12GCController43garbageCollectOnAlternateThreadForDebuggingEb
 __ZN7WebCore12PopupMenuMacC1EPNS_15PopupMenuClientE
 __ZN7WebCore12PrintContext12pagePropertyEPNS_5FrameEPKci

Modified: trunk/Source/WebCore/bindings/js/GCController.cpp (115287 => 115288)


--- trunk/Source/WebCore/bindings/js/GCController.cpp	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/WebCore/bindings/js/GCController.cpp	2012-04-26 05:08:06 UTC (rev 115288)
@@ -49,26 +49,20 @@
 }
 
 GCController::GCController()
-    : m_GCTimer(this, &GCController::gcTimerFired)
 {
 }
 
 void GCController::garbageCollectSoon()
 {
-    if (!m_GCTimer.isActive())
-        m_GCTimer.startOneShot(0);
+    JSLock lock(SilenceAssertionsOnly);
+    JSDOMWindow::commonJSGlobalData()->heap.reportAbandonedObjectGraph();
 }
 
-void GCController::gcTimerFired(Timer<GCController>*)
-{
-    collect(0);
-}
-
 void GCController::garbageCollectNow()
 {
     JSLock lock(SilenceAssertionsOnly);
     if (!JSDOMWindow::commonJSGlobalData()->heap.isBusy())
-        collect(0);
+        JSDOMWindow::commonJSGlobalData()->heap.collectAllGarbage();
 }
 
 void GCController::garbageCollectOnAlternateThreadForDebugging(bool waitUntilDone)

Modified: trunk/Source/WebCore/bindings/js/GCController.h (115287 => 115288)


--- trunk/Source/WebCore/bindings/js/GCController.h	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/WebCore/bindings/js/GCController.h	2012-04-26 05:08:06 UTC (rev 115288)
@@ -26,7 +26,8 @@
 #ifndef GCController_h
 #define GCController_h
 
-#include "Timer.h"
+#include <wtf/FastAllocBase.h>
+#include <wtf/Noncopyable.h>
 
 namespace WebCore {
 
@@ -44,9 +45,6 @@
 
     private:
         GCController(); // Use gcController() instead
-        void gcTimerFired(Timer<GCController>*);
-        
-        Timer<GCController> m_GCTimer;
     };
 
     // Function to obtain the global GC controller.

Modified: trunk/Source/WebCore/bindings/js/ScriptProfiler.cpp (115287 => 115288)


--- trunk/Source/WebCore/bindings/js/ScriptProfiler.cpp	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/WebCore/bindings/js/ScriptProfiler.cpp	2012-04-26 05:08:06 UTC (rev 115288)
@@ -43,7 +43,7 @@
 
 void ScriptProfiler::collectGarbage()
 {
-    gcController().garbageCollectNow();
+    gcController().garbageCollectSoon();
 }
 
 ScriptObject ScriptProfiler::objectByHeapObjectId(unsigned)

Modified: trunk/Source/WebKit2/ChangeLog (115287 => 115288)


--- trunk/Source/WebKit2/ChangeLog	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/WebKit2/ChangeLog	2012-04-26 05:08:06 UTC (rev 115288)
@@ -1,3 +1,14 @@
+2012-04-25  Mark Hahnenberg  <mhahnenb...@apple.com>
+
+        WebCore shouldn't call collectAllGarbage directly
+        https://bugs.webkit.org/show_bug.cgi?id=84897
+
+        Reviewed by Geoffrey Garen.
+
+        * WebProcess/WebProcess.cpp:
+        (WebKit::WebProcess::didClose): Changed to call garbageCollectSoon. This is the 
+        function that causes us to do so much collection on page navigation.
+
 2012-04-25  Beth Dakin  <bda...@apple.com>
 
         https://bugs.webkit.org/show_bug.cgi?id=84909

Modified: trunk/Source/WebKit2/WebProcess/WebProcess.cpp (115287 => 115288)


--- trunk/Source/WebKit2/WebProcess/WebProcess.cpp	2012-04-26 05:07:31 UTC (rev 115287)
+++ trunk/Source/WebKit2/WebProcess/WebProcess.cpp	2012-04-26 05:08:06 UTC (rev 115288)
@@ -686,7 +686,7 @@
         pages[i]->close();
     pages.clear();
 
-    gcController().garbageCollectNow();
+    gcController().garbageCollectSoon();
     memoryCache()->setDisabled(true);
 #endif    
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to