Bug 99884 – [V8] Vastly simplify V8GCController's NodeVisitor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

[V8] Vastly simplify V8GCController's NodeVisitor

[Adam Barth]

Created attachment 169718 [details]
Patch

[Adam Barth]

I didn't mention this in the ChangeLog, but I also got rid of an entire memcpy.

[Eric Seidel (no email)]

Comment on attachment 169718 [details]
Patch

On heavy-GC tests, this should show a speed win, no?

[Kentaro Hara]

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?

[Adam Barth]

(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.

[Adam Barth]

> On heavy-GC tests, this should show a speed win, no?

Yes.  We can try it on Kentaro's createElement case if you like.

[Eric Seidel (no email)]

I would like to see the perf results if possible.  On whatever benchmark you like of course.

[Adam Barth]

Created attachment 169779 [details]
Updated patch with new PerformanceTests

[Adam Barth]

Created attachment 169780 [details]
perf test results (first five are with patch; last five are without)

[Adam Barth]

Created attachment 169781 [details]
Updated patch with new PerformanceTests

[Adam Barth]

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.

[Adam Barth]

Created attachment 169783 [details]
Patch

[Adam Barth]

Comment on attachment 169783 [details]
Patch

Looks like I broke some tests.  :(

[Eric Seidel (no email)]

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?

[Adam Barth]

(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.

[Adam Barth]

Created attachment 169786 [details]
Patch

[Adam Barth]

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.

[Kentaro Hara]

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.

[Adam Barth]

Created attachment 169943 [details]
Patch for landing

[Adam Barth]

Created attachment 169944 [details]
Patch for landing

[Adam Barth]

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.  :)

[WebKit Review Bot]

Comment on attachment 169944 [details]
Patch for landing

Clearing flags on attachment: 169944

Committed r132114: <http://trac.webkit.org/changeset/132114>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.