- Revision
- 141983
- Author
- hara...@chromium.org
- Date
- 2013-02-06 03:40:49 -0800 (Wed, 06 Feb 2013)
Log Message
[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):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (141982 => 141983)
--- trunk/Source/WebCore/ChangeLog 2013-02-06 11:32:16 UTC (rev 141982)
+++ trunk/Source/WebCore/ChangeLog 2013-02-06 11:40:49 UTC (rev 141983)
@@ -1,3 +1,62 @@
+2013-02-04 Kentaro Hara <hara...@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 Mihai Maerean <mmaer...@adobe.com>
[CSSRegions] Assertion failure in Node::detach (!renderer || renderer->inRenderFlowThread())
Modified: trunk/Source/WebCore/bindings/v8/DOMDataStore.h (141982 => 141983)
--- trunk/Source/WebCore/bindings/v8/DOMDataStore.h 2013-02-06 11:32:16 UTC (rev 141982)
+++ trunk/Source/WebCore/bindings/v8/DOMDataStore.h 2013-02-06 11:40:49 UTC (rev 141983)
@@ -164,13 +164,6 @@
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);
Modified: trunk/Source/WebCore/bindings/v8/DOMWrapperWorld.h (141982 => 141983)
--- trunk/Source/WebCore/bindings/v8/DOMWrapperWorld.h 2013-02-06 11:32:16 UTC (rev 141982)
+++ trunk/Source/WebCore/bindings/v8/DOMWrapperWorld.h 2013-02-06 11:40:49 UTC (rev 141983)
@@ -63,6 +63,7 @@
#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 @@
#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
Modified: trunk/Source/WebCore/bindings/v8/V8Binding.h (141982 => 141983)
--- trunk/Source/WebCore/bindings/v8/V8Binding.h 2013-02-06 11:32:16 UTC (rev 141982)
+++ trunk/Source/WebCore/bindings/v8/V8Binding.h 2013-02-06 11:40:49 UTC (rev 141983)
@@ -437,13 +437,31 @@
// 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
// is disabled and it returns true.
bool handleOutOfMemory();
Modified: trunk/Source/WebCore/bindings/v8/V8DOMWindowShell.cpp (141982 => 141983)
--- trunk/Source/WebCore/bindings/v8/V8DOMWindowShell.cpp 2013-02-06 11:32:16 UTC (rev 141982)
+++ trunk/Source/WebCore/bindings/v8/V8DOMWindowShell.cpp 2013-02-06 11:40:49 UTC (rev 141983)
@@ -208,13 +208,13 @@
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()) {
Modified: trunk/Source/WebCore/bindings/v8/V8GCController.cpp (141982 => 141983)
--- trunk/Source/WebCore/bindings/v8/V8GCController.cpp 2013-02-06 11:32:16 UTC (rev 141982)
+++ trunk/Source/WebCore/bindings/v8/V8GCController.cpp 2013-02-06 11:40:49 UTC (rev 141983)
@@ -178,18 +178,116 @@
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 WrapperVisitor(v8::Isolate* isolate)
+ 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 MajorGCWrapperVisitor(v8::Isolate* isolate)
+ : m_isolate(isolate)
+ {
+ }
+
+ virtual void VisitPersistentHandle(v8::Persistent<v8::Value> value, uint16_t classId) OVERRIDE
+ {
+ ASSERT(value->IsObject());
+ v8::Persistent<v8::Object> wrapper = v8::Persistent<v8::Object>::Cast(value);
+
if (classId != v8DOMNodeClassId && classId != v8DOMObjectClassId)
return;
@@ -248,105 +346,28 @@
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();
+
+ // A minor GC can handle the main world only.
+ DOMWrapperWorld* world = worldForEnteredContextWithoutContextCheck();
+ if (world && world->isMainWorld()) {
+ v8::HandleScope scope;
+
+ MinorGCWrapperVisitor visitor(isolate);
+ v8::V8::VisitHandlesForPartialDependence(isolate, &visitor);
+ visitor.notifyFinished();
}
}
@@ -358,7 +379,7 @@
v8::Isolate* isolate = v8::Isolate::GetCurrent();
v8::HandleScope scope;
- WrapperVisitor visitor(isolate);
+ MajorGCWrapperVisitor visitor(isolate);
v8::V8::VisitHandlesWithClassIds(&visitor);
visitor.notifyFinished();
Modified: trunk/Source/WebCore/bindings/v8/V8GCController.h (141982 => 141983)
--- trunk/Source/WebCore/bindings/v8/V8GCController.h 2013-02-06 11:32:16 UTC (rev 141982)
+++ trunk/Source/WebCore/bindings/v8/V8GCController.h 2013-02-06 11:40:49 UTC (rev 141983)
@@ -52,7 +52,6 @@
static void collectGarbage();
static Node* opaqueRootForGC(Node*, v8::Isolate*);
- static void didCreateWrapperForNode(Node*);
private:
static Vector<Node*>* m_edenNodes;
