[V8] Vastly simplify V8GCController's NodeVisitor
Created attachment 169718 [details] Patch
I didn't mention this in the ChangeLog, but I also got rid of an entire memcpy.
Comment on attachment 169718 [details] Patch On heavy-GC tests, this should show a speed win, no?
Comment on attachment 169718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169718&action=review Looks good at a first glance. Let me take a detailed look later. > Source/WebCore/bindings/v8/V8GCController.cpp:136 > + // FIXME: What about <audio>? Are you caring about that <audio> is an active Node?
(In reply to comment #4) > (From update of attachment 169718 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169718&action=review > > Looks good at a first glance. Let me take a detailed look later. > > > Source/WebCore/bindings/v8/V8GCController.cpp:136 > > + // FIXME: What about <audio>? > > Are you caring about that <audio> is an active Node? Oh, you're right. <audio> inherits from HTMLMediaElement. I'll remove the comment.
> On heavy-GC tests, this should show a speed win, no? Yes. We can try it on Kentaro's createElement case if you like.
I would like to see the perf results if possible. On whatever benchmark you like of course.
Created attachment 169779 [details] Updated patch with new PerformanceTests
Created attachment 169780 [details] perf test results (first five are with patch; last five are without)
Created attachment 169781 [details] Updated patch with new PerformanceTests
Created attachment 169782 [details] gc-mini-tree (first three are with patch; last three are without) Just for fun, I added a third PerformanceTest with a workload that shows the new algorithm running 20% faster than the old algorithm.
Created attachment 169783 [details] Patch
Comment on attachment 169783 [details] Patch Looks like I broke some tests. :(
Comment on attachment 169783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169783&action=review > Source/WebCore/bindings/v8/V8GCController.cpp:136 > + // FIXME: What about <audio>? What's this about? > Source/WebCore/bindings/v8/V8GCController.cpp:147 > + while (Node* parent = node->parentOrHostNode()) > + node = parent; Don't we already have a function for this? > Source/WebCore/bindings/v8/V8GCController.cpp:170 > +bool operator<(const ImplicitReference& lhs, const ImplicitReference& rhs) > +{ > + return lhs.root() < rhs.root(); I would have named these "left" and "right" > Source/WebCore/bindings/v8/V8GCController.cpp:208 > + currentGroup.shrink(0); Why? And if so, why not have the Vector declared in the for loop block?
(In reply to comment #14) > (From update of attachment 169783 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169783&action=review > > > Source/WebCore/bindings/v8/V8GCController.cpp:136 > > + // FIXME: What about <audio>? > > What's this about? Kentaro pointed out in Comment #4 that <audio> is fine because it's a subclass of HTMLMediaElement, which is an ActiveDOMObject. > > Source/WebCore/bindings/v8/V8GCController.cpp:147 > > + while (Node* parent = node->parentOrHostNode()) > > + node = parent; > > Don't we already have a function for this? Not that I know of. > > Source/WebCore/bindings/v8/V8GCController.cpp:170 > > +bool operator<(const ImplicitReference& lhs, const ImplicitReference& rhs) > > +{ > > + return lhs.root() < rhs.root(); > > I would have named these "left" and "right" > > > Source/WebCore/bindings/v8/V8GCController.cpp:208 > > + currentGroup.shrink(0); > > Why? And if so, why not have the Vector declared in the for loop block? That's the trick that saves 20% on gc-mini-tree. If we declared the vector in the for loop block, we'd have to malloc it every time. Using shrink lets us re-use the buffer for each iteration.
Created attachment 169786 [details] Patch
I should re-run the benchmarks with the updated patch. I'm sure it's not a regression. I'm just not sure if mini-tree will show the same improvement.
Comment on attachment 169786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169786&action=review Looks OK. Please just re-run the benchmarks and verify the speed-up. > PerformanceTests/ChangeLog:12 > + * Bindings/gc-forest.html: Added. > + * Bindings/gc-mini-tree.html: Added. > + * Bindings/gc-tree.html: Added. It looks like you missed adding these tests.
Created attachment 169943 [details] Patch for landing
Created attachment 169944 [details] Patch for landing
Created attachment 169945 [details] Perf test results (first five are without patch; second five are with patch) Here's a summary of the perf tests results (average of five runs): gc-forest: 1.14% better gc-mini-tree: 5.09% better gc-tree: 4.60% better The performance improvements are icing on this patch (in my view). The main thing I wanted to do here is to delete the nutty code. :)
Comment on attachment 169944 [details] Patch for landing Clearing flags on attachment: 169944 Committed r132114: <http://trac.webkit.org/changeset/132114>
All reviewed patches have been landed. Closing bug.