Changeset 141548 in webkit

Timestamp:

Feb 1, 2013 12:27:10 AM (11 years ago)

Author:

haraken@chromium.org

Message:

[V8] Remove V8GCController::m_edenNodes
​https://bugs.webkit.org/show_bug.cgi?id=108579

Reviewed by Adam Barth.

Currently V8GCController::m_edenNodes stores a list of nodes whose
wrappers have been created since the latest GC. The reason why we
needed m_edenNodes is that there was no way to know a list of wrappers
in the new space of V8. By using m_edenNodes, we had been approximating
'wrappers in the new space' by 'wrappers that have been created since
the latest GC'.

Now V8 provides VisitHandlesForPartialDependence(), with which WebKit
can know a list of wrappers in the new space. By using the API, we can
remove V8GCController::m_edenNodes. The benefit is that (1) we no longer
need to keep m_edenNodes and that (2) it enables more precise minor
DOM GC (Remember that m_edenNodes was just an approximation).

Performance benchmark: ​https://bugs.webkit.org/attachment.cgi?id=185940
The benchmark runs 300 iterations, each of which creates 100000 elements.
The benchmark measures average, min, median, max and stdev of execution times
of the 300 iterations. This will tell us the worst-case overhead of this change.

Before:

mean=59.91ms, min=39ms, median=42ms, max=258ms, stdev=47.48ms

After:

mean=58.75ms, min=35ms, median=41ms, max=250ms, stdev=47.32ms

As shown above, I couldn't observe any performance regression.

No tests. No change in behavior.

• bindings/v8/DOMDataStore.h:

(WebCore::DOMDataStore::setWrapperInObject):

• bindings/v8/V8GCController.cpp:

(WebCore::gcTree):
(WebCore):
(MinorGCWrapperVisitor):
(WebCore::MinorGCWrapperVisitor::notifyFinished):
(WebCore::V8GCController::gcPrologue):
(WebCore::V8GCController::minorGCPrologue):
(WebCore::V8GCController::majorGCPrologue):

• bindings/v8/V8GCController.h:

(V8GCController):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• bindings/v8/DOMDataStore.h (modified) (1 diff)
• bindings/v8/V8GCController.cpp (modified) (5 diffs)
• bindings/v8/V8GCController.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-01  Kentaro Hara  <haraken@chromium.org>
+
+        [V8] Remove V8GCController::m_edenNodes
+        https://bugs.webkit.org/show_bug.cgi?id=108579
+
+        Reviewed by Adam Barth.
+
+        Currently V8GCController::m_edenNodes stores a list of nodes whose
+        wrappers have been created since the latest GC. The reason why we
+        needed m_edenNodes is that there was no way to know a list of wrappers
+        in the new space of V8. By using m_edenNodes, we had been approximating
+        'wrappers in the new space' by 'wrappers that have been created since
+        the latest GC'.
+
+        Now V8 provides VisitHandlesForPartialDependence(), with which WebKit
+        can know a list of wrappers in the new space. By using the API, we can
+        remove V8GCController::m_edenNodes. The benefit is that (1) we no longer
+        need to keep m_edenNodes and that (2) it enables more precise minor
+        DOM GC (Remember that m_edenNodes was just an approximation).
+
+        Performance benchmark: https://bugs.webkit.org/attachment.cgi?id=185940
+        The benchmark runs 300 iterations, each of which creates 100000 elements.
+        The benchmark measures average, min, median, max and stdev of execution times
+        of the 300 iterations. This will tell us the worst-case overhead of this change.
+
+        Before:
+          mean=59.91ms, min=39ms, median=42ms, max=258ms, stdev=47.48ms
+
+        After:
+          mean=58.75ms, min=35ms, median=41ms, max=250ms, stdev=47.32ms
+
+        As shown above, I couldn't observe any performance regression.
+
+        No tests. No change in behavior.
+
+        * bindings/v8/DOMDataStore.h:
+        (WebCore::DOMDataStore::setWrapperInObject):
+        * bindings/v8/V8GCController.cpp:
+        (WebCore::gcTree):
+        (WebCore):
+        (MinorGCWrapperVisitor):
+        (WebCore::MinorGCWrapperVisitor::notifyFinished):
+        (WebCore::V8GCController::gcPrologue):
+        (WebCore::V8GCController::minorGCPrologue):
+        (WebCore::V8GCController::majorGCPrologue):
+        * bindings/v8/V8GCController.h:
+        (V8GCController):
+
 2013-02-01  Vsevolod Vlasov  <vsevik@chromium.org>
 

trunk/Source/WebCore/bindings/v8/DOMDataStore.h

@@ -165 +165 @@
         wrapper.MakeWeak(object, weakCallback);
     }
-    static void setWrapperInObject(Node* object, v8::Persistent<v8::Object> wrapper)
-    {
-        ASSERT(object->wrapper().IsEmpty());
-        object->setWrapper(wrapper);
-        V8GCController::didCreateWrapperForNode(object);
-        wrapper.MakeWeak(static_cast<ScriptWrappable*>(object), weakCallback);
-    }
 
     static void weakCallback(v8::Persistent<v8::Value>, void* context);

trunk/Source/WebCore/bindings/v8/V8GCController.cpp

@@ -179 +179 @@
 }
 
-class WrapperVisitor : public v8::PersistentHandleVisitor {
+static void gcTree(Node* startNode)
+{
+    Vector<v8::Persistent<v8::Value>, initialNodeVectorSize> newSpaceWrappers;
+
+    // We traverse a DOM tree in the DFS order starting from startNode.
+    // The traversal order does not matter for correctness but does matter for performance.
+    Node* node = startNode;
+    // To make each minor GC time bounded, we might need to give up
+    // traversing at some point for a large DOM tree. That being said,
+    // I could not observe the need even in pathological test cases.
+    do {
+        ASSERT(node);
+        if (!node->wrapper().IsEmpty()) {
+            // FIXME: Remove the special handling for image elements.
+            // The same special handling is in V8GCController::opaqueRootForGC().
+            // Maybe should image elements be active DOM nodes?
+            // See https://code.google.com/p/chromium/issues/detail?id=164882
+            if (!node->isV8CollectableDuringMinorGC() || (node->hasTagName(HTMLNames::imgTag) && static_cast<HTMLImageElement*>(node)->hasPendingActivity())) {
+                // This node is not in the new space of V8. This indicates that
+                // the minor GC cannot anyway judge reachability of this DOM tree.
+                // Thus we give up traversing the DOM tree.
+                return;
+            }
+            node->setV8CollectableDuringMinorGC(false);
+            newSpaceWrappers.append(node->wrapper());
+        }
+        if (node->firstChild()) {
+            node = node->firstChild();
+            continue;
+        }
+        while (!node->nextSibling()) {
+            if (!node->parentNode())
+                break;
+            node = node->parentNode();
+        }
+        if (node->parentNode())
+            node = node->nextSibling();
+    } while (node != startNode);
+
+    // We completed the DOM tree traversal. All wrappers in the DOM tree are
+    // stored in newSpaceWrappers and are expected to exist in the new space of V8.
+    // We report those wrappers to V8 as an object group.
+    for (size_t i = 0; i < newSpaceWrappers.size(); i++)
+        newSpaceWrappers[i].MarkPartiallyDependent();
+    if (newSpaceWrappers.size() > 0)
+        v8::V8::AddObjectGroup(&newSpaceWrappers[0], newSpaceWrappers.size());
+}
+
+// Regarding a minor GC algorithm for DOM nodes, see this document:
+// https://docs.google.com/a/google.com/presentation/d/1uifwVYGNYTZDoGLyCb7sXa7g49mWNMW2gaWvMN5NLk8/edit#slide=id.p
+class MinorGCWrapperVisitor : public v8::PersistentHandleVisitor {
+public:
+    virtual void VisitPersistentHandle(v8::Persistent<v8::Value> value, uint16_t classId) OVERRIDE
+    {
+        // A minor DOM GC can collect only Nodes.
+        if (classId != v8DOMNodeClassId)
+            return;
+
+        // To make minor GC cycle time bounded, we limit the number of wrappers handled
+        // by each minor GC cycle to 10000. This value was selected so that the minor
+        // GC cycle time is bounded to 20 ms in a case where the new space size
+        // is 16 MB and it is full of wrappers (which is almost the worst case).
+        // Practically speaking, as far as I crawled real web applications,
+        // the number of wrappers handled by each minor GC cycle is at most 3000.
+        // So this limit is mainly for pathological micro benchmarks.
+        const unsigned wrappersHandledByEachMinorGC = 10000;
+        if (m_nodesInNewSpace.size() >= wrappersHandledByEachMinorGC)
+            return;
+
+        ASSERT(value->IsObject());
+        v8::Persistent<v8::Object> wrapper = v8::Persistent<v8::Object>::Cast(value);
+        ASSERT(V8DOMWrapper::maybeDOMWrapper(value));
+        ASSERT(V8Node::HasInstance(wrapper));
+        Node* node = V8Node::toNative(wrapper);
+        m_nodesInNewSpace.append(node);
+        node->setV8CollectableDuringMinorGC(true);
+    }
+
+    void notifyFinished()
+    {
+        for (size_t i = 0; i < m_nodesInNewSpace.size(); i++) {
+            ASSERT(!m_nodesInNewSpace[i]->wrapper().IsEmpty());
+            if (m_nodesInNewSpace[i]->isV8CollectableDuringMinorGC()) // This branch is just for performance.
+                gcTree(m_nodesInNewSpace[i]);
+        }
+    }
+
+private:
+    Vector<Node*> m_nodesInNewSpace;
+};
+
+class MajorGCWrapperVisitor : public v8::PersistentHandleVisitor {
 public:
     virtual void VisitPersistentHandle(v8::Persistent<v8::Value> value, uint16_t classId) OVERRIDE
@@ -242 +333 @@
 };
 
-// Regarding a minor GC algorithm for DOM nodes, see this document:
-// https://docs.google.com/a/google.com/presentation/d/1uifwVYGNYTZDoGLyCb7sXa7g49mWNMW2gaWvMN5NLk8/edit#slide=id.p
-//
-// m_edenNodes stores nodes that have wrappers that have been created since the last minor/major GC.
-Vector<Node*>* V8GCController::m_edenNodes = 0;
-
-static void gcTree(Node* startNode)
-{
-    Vector<v8::Persistent<v8::Value>, initialNodeVectorSize> newSpaceWrappers;
-
-    // We traverse a DOM tree in the DFS order starting from startNode.
-    // The traversal order does not matter for correctness but does matter for performance.
-    Node* node = startNode;
-    // To make each minor GC time bounded, we might need to give up
-    // traversing at some point for a large DOM tree. That being said,
-    // I could not observe the need even in pathological test cases.
-    do {
-        ASSERT(node);
-        if (!node->wrapper().IsEmpty()) {
-            // FIXME: Remove the special handling for image elements.
-            // The same special handling is in V8GCController::opaqueRootForGC().
-            // Maybe should image elements be active DOM nodes?
-            // See https://code.google.com/p/chromium/issues/detail?id=164882
-            if (!node->isV8CollectableDuringMinorGC() || (node->hasTagName(HTMLNames::imgTag) && static_cast<HTMLImageElement*>(node)->hasPendingActivity())) {
-                // The fact that we encounter a node that is not in the Eden space
-                // implies that its wrapper might be in the old space of V8.
-                // This indicates that the minor GC cannot anyway judge reachability
-                // of this DOM tree. Thus we give up traversing the DOM tree.
-                return;
-            }
-            // A once traversed node is removed from the Eden space.
-            node->setV8CollectableDuringMinorGC(false);
-            newSpaceWrappers.append(node->wrapper());
-        }
-        if (node->firstChild()) {
-            node = node->firstChild();
-            continue;
-        }
-        while (!node->nextSibling()) {
-            if (!node->parentNode())
-                break;
-            node = node->parentNode();
-        }
-        if (node->parentNode())
-            node = node->nextSibling();
-    } while (node != startNode);
-
-    // We completed the DOM tree traversal. All wrappers in the DOM tree are
-    // stored in newSpaceWrappers and are expected to exist in the new space of V8.
-    // We report those wrappers to V8 as an object group.
-    for (size_t i = 0; i < newSpaceWrappers.size(); i++)
-        newSpaceWrappers[i].MarkPartiallyDependent();
-    if (newSpaceWrappers.size() > 0)
-        v8::V8::AddObjectGroup(&newSpaceWrappers[0], newSpaceWrappers.size());
-}
-
-void V8GCController::didCreateWrapperForNode(Node* node)
-{
-    // To make minor GC cycle time bounded, we limit the number of wrappers handled
-    // by each minor GC cycle to 10000. This value was selected so that the minor
-    // GC cycle time is bounded to 20 ms in a case where the new space size
-    // is 16 MB and it is full of wrappers (which is almost the worst case).
-    // Practically speaking, as far as I crawled real web applications,
-    // the number of wrappers handled by each minor GC cycle is at most 3000.
-    // So this limit is mainly for pathological micro benchmarks.
-    const unsigned wrappersHandledByEachMinorGC = 10000;
-    ASSERT(!node->wrapper().IsEmpty());
-    if (!m_edenNodes)
-        m_edenNodes = adoptPtr(new Vector<Node*>).leakPtr();
-    if (m_edenNodes->size() <= wrappersHandledByEachMinorGC) {
-        // A node of a newly created wrapper is put into the Eden space.
-        m_edenNodes->append(node);
-        node->setV8CollectableDuringMinorGC(true);
-    }
-}
-
 void V8GCController::gcPrologue(v8::GCType type, v8::GCCallbackFlags flags)
 {
@@ -324 +339 @@
     else if (type == v8::kGCTypeMarkSweepCompact)
         majorGCPrologue();
-
-    if (isMainThreadOrGCThread() && m_edenNodes) {
-        // The Eden space is cleared at every minor/major GC.
-        m_edenNodes->clear();
-    }
 }
 
@@ -335 +345 @@
     TRACE_EVENT_BEGIN0("v8", "GC");
 
-    if (isMainThreadOrGCThread() && m_edenNodes) {
-        for (size_t i = 0; i < m_edenNodes->size(); i++) {
-            ASSERT(!m_edenNodes->at(i)->wrapper().IsEmpty());
-            if (m_edenNodes->at(i)->isV8CollectableDuringMinorGC()) // This branch is just for performance.
-                gcTree(m_edenNodes->at(i));
-        }
-    }
+    v8::HandleScope scope;
+
+    MinorGCWrapperVisitor visitor;
+    v8::V8::VisitHandlesForPartialDependence(v8::Isolate::GetCurrent(), &visitor);
+    visitor.notifyFinished();
 }
 
@@ -351 +359 @@
     v8::HandleScope scope;
 
-    WrapperVisitor visitor;
+    MajorGCWrapperVisitor visitor;
     v8::V8::VisitHandlesWithClassIds(&visitor);
     visitor.notifyFinished();

trunk/Source/WebCore/bindings/v8/V8GCController.h

@@ -53 +53 @@
 
     static Node* opaqueRootForGC(Node*);
-    static void didCreateWrapperForNode(Node*);
 
 private: