Use traverseNextNode in gcTree
Created attachment 175045 [details] Patch
In Bug 98725 haraken said that this traversal was different than traverseNextNode but near as I can tell it's identical to what traverseNextNode(stayWithin) does and that method is inline so it doesn't make any sense that it would be faster to manually inline it instead of letting the compiler do it.
This looks like a patch for haraken to review.
Created attachment 175050 [details] Patch
Comment on attachment 175050 [details] Patch 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.)
(In reply to comment #5) > (From update of attachment 175050 [details]) > 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.
(In reply to comment #6) > ... > > I also don't understand why this would be more instructions, it's an inline method call. It would produce essentially identical code. Actually I see that Node::traverseNextAncestorSibling is not inline, still this feels like premature optimization.
Comment on attachment 175050 [details] Patch 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".
(In reply to comment #8) > .. > > 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". Sure.