[webkit-reviews] review granted: [Bug 98725] [V8] DOM wrapper objects should be collected in minor GC cycles : [Attachment 172758] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 98725: [V8] DOM wrapper objects should be collected in minor GC cycles
https://bugs.webkit.org/show_bug.cgi?id=98725

Attachment 172758: Patch
https://bugs.webkit.org/attachment.cgi?id=172758&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=172758&action=review


> Source/WebCore/bindings/v8/IntrusiveDOMWrapperMap.h:51
> +	   V8GCController::newWrapperBorn(key);

Oh noes!  I'm about to delete this function in order to generalize
IntrusiveDOMWrapperMap to all ScriptWrappable objects.	We'll figure something
out.

> Source/WebCore/bindings/v8/V8GCController.cpp:238
> +Vector<Node*> V8GCController::m_edenNodes;

Does this create a static initializer?	Typically we put these sorts of things
inside functions to initialize them lazily.

> Source/WebCore/bindings/v8/V8GCController.cpp:242
> +    Vector<v8::Persistent<v8::Value>, 11> newSpaceWrappers;

11 -> ??

> Source/WebCore/bindings/v8/V8GCController.cpp:255
> +    while (true) {
> +	   ASSERT(node);
> +	   if (traverseStarted && node == startNode)
> +	       break;
> +	   traverseStarted = true;

Why not just use a do { ... } while loop?

> Source/WebCore/bindings/v8/V8GCController.cpp:265
> +	       // A once traversed node is evacuated from the Eden space.

evacuated -> removed

> Source/WebCore/bindings/v8/V8GCController.cpp:280
> +	   if (node->firstChild()) {
> +	       node = node->firstChild();
> +	       continue;
> +	   }
> +	   while (!node->nextSibling()) {
> +	       if (!node->parentNode())
> +		   break;
> +	       node = node->parentNode();
> +	   }
> +	   if (node->parentNode())
> +	       node = node->nextSibling();

Isn't there a node->traverseNext function that does this work for us?

> Source/WebCore/bindings/v8/V8GCController.cpp:286
> +    for (unsigned i = 0; i < newSpaceWrappers.size(); i++)

unsigned -> size_t

> Source/WebCore/bindings/v8/V8GCController.cpp:302
> +    if (m_edenNodes.size() <= 10000) {

10000 -> can we name this constant?

> Source/WebCore/bindings/v8/V8GCController.cpp:311
> +    minorGCPrologue();

We do the minorGCPrologue even for major GCs?

> Source/WebCore/bindings/v8/V8GCController.cpp:324
> +    for (unsigned i = 0; i < m_edenNodes.size(); i++) {

unsigned -> size_t

> Source/WebCore/bindings/v8/V8GCController.cpp:325
> +	   ASSERT(m_edenNodes[i]->wrapper());

ASSERT(!m_edenNodes[i]->wrapper().IsEmpty())