Changeset 141577 in webkit

Timestamp:

Feb 1, 2013 3:49:35 AM (11 years ago)

Author:

haraken@chromium.org

Message:

Unreviewed, rolling out r141548.
​http://trac.webkit.org/changeset/141548
​https://bugs.webkit.org/show_bug.cgi?id=108579

userscript-plugin-document.html is flaky

• bindings/v8/DOMDataStore.h:

(WebCore::DOMDataStore::setWrapperInObject):

• bindings/v8/V8GCController.cpp:

(WebCore):
(WebCore::gcTree):
(WebCore::V8GCController::didCreateWrapperForNode):
(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>
+
+        Unreviewed, rolling out r141548.
+        http://trac.webkit.org/changeset/141548
+        https://bugs.webkit.org/show_bug.cgi?id=108579
+
+        userscript-plugin-document.html is flaky
+
+        * bindings/v8/DOMDataStore.h:
+        (WebCore::DOMDataStore::setWrapperInObject):
+        * bindings/v8/V8GCController.cpp:
+        (WebCore):
+        (WebCore::gcTree):
+        (WebCore::V8GCController::didCreateWrapperForNode):
+        (WebCore::V8GCController::gcPrologue):
+        (WebCore::V8GCController::minorGCPrologue):
+        (WebCore::V8GCController::majorGCPrologue):
+        * bindings/v8/V8GCController.h:
+        (V8GCController):
+
 2013-02-01  Andrey Adaikin  <aandrey@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 @@
 }
 
-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 {
+class WrapperVisitor : public v8::PersistentHandleVisitor {
 public:
     virtual void VisitPersistentHandle(v8::Persistent<v8::Value> value, uint16_t classId) OVERRIDE
@@ -333 +242 @@
 };
 
+// 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)
 {
@@ -339 +324 @@
     else if (type == v8::kGCTypeMarkSweepCompact)
         majorGCPrologue();
+
+    if (isMainThreadOrGCThread() && m_edenNodes) {
+        // The Eden space is cleared at every minor/major GC.
+        m_edenNodes->clear();
+    }
 }
 
@@ -345 +335 @@
     TRACE_EVENT_BEGIN0("v8", "GC");
 
-    v8::HandleScope scope;
-
-    MinorGCWrapperVisitor visitor;
-    v8::V8::VisitHandlesForPartialDependence(v8::Isolate::GetCurrent(), &visitor);
-    visitor.notifyFinished();
+    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));
+        }
+    }
 }
 
@@ -359 +351 @@
     v8::HandleScope scope;
 
-    MajorGCWrapperVisitor visitor;
+    WrapperVisitor 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: