[V8] Remove V8GCController::m_edenNodes and make minor DOM GC more precise
authorharaken@chromium.org <haraken@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Feb 2013 08:42:13 +0000 (08:42 +0000)
committerharaken@chromium.org <haraken@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Feb 2013 08:42:13 +0000 (08:42 +0000)
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/DOMWrapperWorld.h:
(DOMWrapperWorld):
(WebCore::DOMWrapperWorld::getWorldWithoutContextCheck):
* bindings/v8/V8Binding.h:
(WebCore):
(WebCore::worldForEnteredContextIfIsolated):
(WebCore::worldForEnteredContextWithoutContextCheck):
* bindings/v8/V8DOMWindowShell.cpp:
(WebCore::V8DOMWindowShell::initializeIfNeeded):
* bindings/v8/V8GCController.cpp:
(WebCore::gcTree):
(WebCore):
(MinorGCWrapperVisitor):
(WebCore::MinorGCWrapperVisitor::MinorGCWrapperVisitor):
(WebCore::MinorGCWrapperVisitor::notifyFinished):
(WebCore::MajorGCWrapperVisitor::MajorGCWrapperVisitor):
(WebCore::V8GCController::gcPrologue):
(WebCore::V8GCController::minorGCPrologue):
(WebCore::V8GCController::majorGCPrologue):
* bindings/v8/V8GCController.h:
(V8GCController):

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

Source/WebCore/ChangeLog
Source/WebCore/bindings/v8/DOMDataStore.h
Source/WebCore/bindings/v8/DOMWrapperWorld.h
Source/WebCore/bindings/v8/V8Binding.h
Source/WebCore/bindings/v8/V8DOMWindowShell.cpp
Source/WebCore/bindings/v8/V8GCController.cpp
Source/WebCore/bindings/v8/V8GCController.h

index 75fea11..109b6d9 100644 (file)
@@ -1,3 +1,62 @@
+2013-02-04  Kentaro Hara  <haraken@chromium.org>
+
+        [V8] Remove V8GCController::m_edenNodes and make minor DOM GC more precise
+        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/DOMWrapperWorld.h:
+        (DOMWrapperWorld):
+        (WebCore::DOMWrapperWorld::getWorldWithoutContextCheck):
+        * bindings/v8/V8Binding.h:
+        (WebCore):
+        (WebCore::worldForEnteredContextIfIsolated):
+        (WebCore::worldForEnteredContextWithoutContextCheck):
+        * bindings/v8/V8DOMWindowShell.cpp:
+        (WebCore::V8DOMWindowShell::initializeIfNeeded):
+        * bindings/v8/V8GCController.cpp:
+        (WebCore::gcTree):
+        (WebCore):
+        (MinorGCWrapperVisitor):
+        (WebCore::MinorGCWrapperVisitor::MinorGCWrapperVisitor):
+        (WebCore::MinorGCWrapperVisitor::notifyFinished):
+        (WebCore::MajorGCWrapperVisitor::MajorGCWrapperVisitor):
+        (WebCore::V8GCController::gcPrologue):
+        (WebCore::V8GCController::minorGCPrologue):
+        (WebCore::V8GCController::majorGCPrologue):
+        * bindings/v8/V8GCController.h:
+        (V8GCController):
+
 2013-02-06  Kent Tamura  <tkent@chromium.org>
 
         REGRESSION(r141195): INPUT_MULTIPLE_FIELDS_UI: Space in a placeholder string is removed
index c98b5e4..d72d4a5 100644 (file)
@@ -164,13 +164,6 @@ private:
         object->setWrapper(wrapper);
         wrapper.MakeWeak(isolate, object, weakCallback);
     }
-    static void setWrapperInObject(Node* object, v8::Persistent<v8::Object> wrapper, v8::Isolate* isolate)
-    {
-        ASSERT(object->wrapper().IsEmpty());
-        object->setWrapper(wrapper);
-        V8GCController::didCreateWrapperForNode(object);
-        wrapper.MakeWeak(isolate, static_cast<ScriptWrappable*>(object), weakCallback);
-    }
 
     static void weakCallback(v8::Isolate*, v8::Persistent<v8::Value>, void* context);
 
index d625fe7..8f2a90a 100644 (file)
@@ -63,6 +63,7 @@ public:
 #ifndef NDEBUG
     static void assertContextHasCorrectPrototype(v8::Handle<v8::Context>);
 #endif
+    // FIXME: Rename this method to getWorld().
     static DOMWrapperWorld* isolated(v8::Handle<v8::Context> context)
     {
 #ifndef NDEBUG
@@ -70,6 +71,10 @@ public:
 #endif
         return static_cast<DOMWrapperWorld*>(context->GetAlignedPointerFromEmbedderData(v8ContextIsolatedWorld));
     }
+    static DOMWrapperWorld* getWorldWithoutContextCheck(v8::Handle<v8::Context> context)
+    {
+        return static_cast<DOMWrapperWorld*>(context->GetAlignedPointerFromEmbedderData(v8ContextIsolatedWorld));
+    }
 
     // Associates an isolated world (see above for description) with a security
     // origin. XMLHttpRequest instances used in that world will be considered
index 17efaa2..63ad6d0 100644 (file)
@@ -437,11 +437,29 @@ namespace WebCore {
     // a context, if the window is currently being displayed in the Frame.
     Frame* toFrameIfNotDetached(v8::Handle<v8::Context>);
 
+    // FIXME: Rename this method to worldForEnteredContext().
     inline DOMWrapperWorld* worldForEnteredContextIfIsolated()
     {
-        if (!v8::Context::InContext())
+        v8::Handle<v8::Context> context = v8::Context::GetEntered();
+        if (context.IsEmpty())
             return 0;
-        return DOMWrapperWorld::isolated(v8::Context::GetEntered());
+        return DOMWrapperWorld::isolated(context);
+    }
+
+    // This is a slightly different version of worldForEnteredContext().
+    // The difference is just that worldForEnteredContextWithoutContextCheck()
+    // does not call assertContextHasCorrectPrototype() (which is enabled on
+    // Debug builds only). Because assertContextHasCorrectPrototype() crashes
+    // if it is called when a current context is not completely initialized,
+    // you have to use worldForEnteredContextWithoutContextCheck() if you need
+    // to get a DOMWrapperWorld while a current context is being initialized.
+    // See https://bugs.webkit.org/show_bug.cgi?id=108579#c15 for more details.
+    inline DOMWrapperWorld* worldForEnteredContextWithoutContextCheck()
+    {
+        v8::Handle<v8::Context> context = v8::Context::GetEntered();
+        if (context.IsEmpty())
+            return 0;
+        return DOMWrapperWorld::getWorldWithoutContextCheck(context);
     }
 
     // If the current context causes out of memory, JavaScript setting
index ca60099..35c6d6a 100644 (file)
@@ -208,13 +208,13 @@ bool V8DOMWindowShell::initializeIfNeeded()
     if (m_context.isEmpty())
         return false;
 
+    m_world->setIsolatedWorldField(m_context.get());
+
     bool isMainWorld = m_world->isMainWorld();
 
     v8::Local<v8::Context> context = v8::Local<v8::Context>::New(m_context.get());
     v8::Context::Scope contextScope(context);
 
-    m_world->setIsolatedWorldField(m_context.get());
-
     if (m_global.isEmpty()) {
         m_global.set(context->Global());
         if (m_global.isEmpty()) {
index 282c410..448b222 100644 (file)
@@ -178,9 +178,107 @@ Node* V8GCController::opaqueRootForGC(Node* node, v8::Isolate*)
     return node;
 }
 
-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:
+    explicit MinorGCWrapperVisitor(v8::Isolate* isolate)
+        : m_isolate(isolate)
+    {
+        UNUSED_PARAM(m_isolate);
+    }
+
+    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, m_isolate));
+        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;
+    v8::Isolate* m_isolate;
+};
+
+class MajorGCWrapperVisitor : public v8::PersistentHandleVisitor {
 public:
-    explicit WrapperVisitor(v8::Isolate* isolate)
+    explicit MajorGCWrapperVisitor(v8::Isolate* isolate)
         : m_isolate(isolate)
     {
     }
@@ -248,105 +346,27 @@ private:
     v8::Isolate* m_isolate;
 };
 
-// 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)
 {
     if (type == v8::kGCTypeScavenge)
         minorGCPrologue();
     else if (type == v8::kGCTypeMarkSweepCompact)
         majorGCPrologue();
-
-    if (isMainThreadOrGCThread() && m_edenNodes) {
-        // The Eden space is cleared at every minor/major GC.
-        m_edenNodes->clear();
-    }
 }
 
 void V8GCController::minorGCPrologue()
 {
     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::Isolate* isolate = v8::Isolate::GetCurrent();
+    v8::HandleScope scope;
+
+    // A minor GC can handle the main world only.
+    DOMWrapperWorld* world = worldForEnteredContextWithoutContextCheck();
+    if (world && world->isMainWorld()) {
+        MinorGCWrapperVisitor visitor(isolate);
+        v8::V8::VisitHandlesForPartialDependence(isolate, &visitor);
+        visitor.notifyFinished();
     }
 }
 
@@ -358,7 +378,7 @@ void V8GCController::majorGCPrologue()
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
     v8::HandleScope scope;
 
-    WrapperVisitor visitor(isolate);
+    MajorGCWrapperVisitor visitor(isolate);
     v8::V8::VisitHandlesWithClassIds(&visitor);
     visitor.notifyFinished();
 
index 32d1a08..d3818f8 100644 (file)
@@ -52,7 +52,6 @@ public:
     static void collectGarbage();
 
     static Node* opaqueRootForGC(Node*, v8::Isolate*);
-    static void didCreateWrapperForNode(Node*);
 
 private:
     static Vector<Node*>* m_edenNodes;