Changeset 121316 in webkit

Timestamp:

Jun 26, 2012, 8:44:05 PM (14 years ago)

Author:

ggaren@apple.com

Message:

Reduced (but did not eliminate) use of "berzerker GC"
​https://bugs.webkit.org/show_bug.cgi?id=89237

Reviewed by Gavin Barraclough.

(PART 2)

../JavaScriptCore:

This part turns off "berzerker GC" and turns on incremental shrinking.

• heap/IncrementalSweeper.cpp:

(JSC::IncrementalSweeper::doSweep): Free or shrink after sweeping to
maintain the behavior we used to get from the occasional berzerker GC,
which would run all finalizers and then free or shrink all blocks
synchronously.

• heap/MarkedBlock.h:

(JSC::MarkedBlock::needsSweeping): Sweep zapped blocks, too. It's always
safe to sweep a zapped block (that's the point of zapping), and it's
sometimes profitable. For example, consider this case: Block A does some
allocation (transitioning Block A from Marked to FreeListed), then GC
happens (transitioning Block A to Zapped), then all objects in Block A
are free, then the incremental sweeper visits Block A. If we skipped
Zapped blocks, we'd skip Block A, even though it would be profitable to
run its destructors and free its memory.

• runtime/GCActivityCallback.cpp:

(JSC::DefaultGCActivityCallback::doWork): Don't sweep eagerly; we'll do
this incrementally.

../WebCore:

Don't ASSERT that RootObject's destructor runs and invalidates all
RuntimeObjects before their destructors run.

We don't guarantee this behavior because some RuntimeObjects may already
be garbage by the time RootObject's destructor runs, in which case
RootObject's weak pointers will be NULL, and RootObject will not call
invalidate() on them.

It's been theoretically possible for this ASSERT to fire for a while now.
This patch makes it fire all the time.

Luckily, we only needed the behavior guarded by this ASSERT for WebKit1
in Safari on Windows (cf. ​https://bugs.webkit.org/show_bug.cgi?id=61317),
to handle the way WebKit1 would unload plugin DLLs. If this ever becomes
an issue again, we can fix it by (a) not unloading plugin DLLs,
(b) migrating WebKit1 to the WebKit2 JS-plugin binding model, (c) making
the Instance pointer in a RuntimeObject an indirect pointer through
RootObject, or (c) giving RuntimeObject some sort of special way to
access a zombie weak pointer.

• bridge/runtime_object.cpp:

(JSC::Bindings::RuntimeObject::destroy): ASSERT removed. Anders said so.

Location:

trunk/Source

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/heap/IncrementalSweeper.cpp (modified) (1 diff)
• JavaScriptCore/heap/MarkedBlock.h (modified) (1 diff)
• JavaScriptCore/runtime/GCActivityCallback.cpp (modified) (1 diff)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/bridge/runtime_object.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2012-06-26  Geoffrey Garen  <ggaren@apple.com>
+
+        Reduced (but did not eliminate) use of "berzerker GC"
+        https://bugs.webkit.org/show_bug.cgi?id=89237
+
+        Reviewed by Gavin Barraclough.
+
+        (PART 2)
+
+        This part turns off "berzerker GC" and turns on incremental shrinking.
+
+        * heap/IncrementalSweeper.cpp:
+        (JSC::IncrementalSweeper::doSweep): Free or shrink after sweeping to
+        maintain the behavior we used to get from the occasional berzerker GC,
+        which would run all finalizers and then free or shrink all blocks
+        synchronously.
+
+        * heap/MarkedBlock.h:
+        (JSC::MarkedBlock::needsSweeping): Sweep zapped blocks, too. It's always
+        safe to sweep a zapped block (that's the point of zapping), and it's
+        sometimes profitable. For example, consider this case: Block A does some
+        allocation (transitioning Block A from Marked to FreeListed), then GC
+        happens (transitioning Block A to Zapped), then all objects in Block A
+        are free, then the incremental sweeper visits Block A. If we skipped
+        Zapped blocks, we'd skip Block A, even though it would be profitable to
+        run its destructors and free its memory.
+
+        * runtime/GCActivityCallback.cpp:
+        (JSC::DefaultGCActivityCallback::doWork): Don't sweep eagerly; we'll do
+        this incrementally.
+
 2012-06-26  Filip Pizlo  <fpizlo@apple.com>
 

trunk/Source/JavaScriptCore/heap/IncrementalSweeper.cpp

@@ -79 +79 @@
 
         block->sweep();
+        m_globalData->heap.objectSpace().freeOrShrinkBlock(block);
 
         CFTimeInterval elapsedTime = WTF::monotonicallyIncreasingTime() - sweepBeginTime;

trunk/Source/JavaScriptCore/heap/MarkedBlock.h

@@ -409 +409 @@
     inline bool MarkedBlock::needsSweeping()
     {
-        return m_state == Marked;
+        return m_state == Marked || m_state == Zapped;
     }
 

trunk/Source/JavaScriptCore/runtime/GCActivityCallback.cpp

@@ -76 +76 @@
     }
 #endif
-    heap->collectAllGarbage();
+    heap->collect(Heap::DoNotSweep);
 }
     

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2012-06-26  Geoffrey Garen  <ggaren@apple.com>
+
+        Reduced (but did not eliminate) use of "berzerker GC"
+        https://bugs.webkit.org/show_bug.cgi?id=89237
+
+        Reviewed by Gavin Barraclough.
+
+        (PART 2)
+
+        Don't ASSERT that RootObject's destructor runs and invalidates all
+        RuntimeObjects before their destructors run.
+
+        We don't guarantee this behavior because some RuntimeObjects may already
+        be garbage by the time RootObject's destructor runs, in which case
+        RootObject's weak pointers will be NULL, and RootObject will not call
+        invalidate() on them.
+
+        It's been theoretically possible for this ASSERT to fire for a while now.
+        This patch makes it fire all the time.
+
+        Luckily, we only needed the behavior guarded by this ASSERT for WebKit1
+        in Safari on Windows (cf. https://bugs.webkit.org/show_bug.cgi?id=61317),
+        to handle the way WebKit1 would unload plugin DLLs. If this ever becomes
+        an issue again, we can fix it by (a) not unloading plugin DLLs,
+        (b) migrating WebKit1 to the WebKit2 JS-plugin binding model, (c) making
+        the Instance pointer in a RuntimeObject an indirect pointer through
+        RootObject, or (c) giving RuntimeObject some sort of special way to
+        access a zombie weak pointer.
+
+        * bridge/runtime_object.cpp:
+        (JSC::Bindings::RuntimeObject::destroy): ASSERT removed. Anders said so.
+
 2012-06-26  Douglas Stockwell  <dstockwell@chromium.org>
 

trunk/Source/WebCore/bridge/runtime_object.cpp

@@ -52 +52 @@
 void RuntimeObject::destroy(JSCell* cell)
 {
-    RuntimeObject* thisObject = static_cast<RuntimeObject*>(cell);
-    ASSERT(!thisObject->m_instance);
-    thisObject->RuntimeObject::~RuntimeObject();
+    static_cast<RuntimeObject*>(cell)->RuntimeObject::~RuntimeObject();
 }