[webkit-reviews] review granted: [Bug 102717] Use traverseNextNode in gcTree : [Attachment 175050] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has granted  review:
Bug 102717: Use traverseNextNode in gcTree
https://bugs.webkit.org/show_bug.cgi?id=102717

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=175050&action=review


>>> Source/WebCore/bindings/v8/V8GCController.cpp:227
>>> +	 for (Node* node = startNode; node; node->traverseNextNode(startNode))
{
>> 
>> traverseNextNode() returns 0 when it reaches a root node, doesn't it? Here
we need to do the DOM traversal until the traversal gets back to startNode.
>> 
>> Adding a couple of branches will solve the problem. That being said, the
number of if instructions will be fewer in the current code than the code that
uses traverseNextNode() + some additional branches. (I don't know whether it is
really important for performance though.)
> 
> What do you mean by reaches a root note? traverseNextNode(stayWithin) does
exactly what your code does, it traverses down and back up until it reaches the
node you pass into it (startNode).
> 
> I also don't understand why this would be more instructions, it's an inline
method call. It would produce essentially identical code.

Ah, got it. I was misunderstanding traverseNextNode(stayWithin) and
traverseNextNode(). Your patch looks correct.

As for performance, the current code will be a win, though I don't know it's a
meaningful win. Would you confirm that it won't regress performance before
landing? You can use this benchmark:
https://bugs.webkit.org/attachment.cgi?id=169542 and observe minor GC cycle
times by ./out/Release/chrome --js-flags="--trace-gc".