Bug 102297 – [V8] We can shave one instruction off toV8Fast

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

[Adam Barth]

[V8] We can shave one instruction off toV8Fast

[Adam Barth]

Created attachment 174281 [details]
Patch

[Kentaro Hara]

Comment on attachment 174281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174281&action=review

> Source/WebCore/ChangeLog:8
> +        There's no reason to store info.Holder in a local variable. Calling

Just in case, would you check the performance in Bindings/first-child.html ? In the DOM binding benchmark suite in v8-internal (which is a super micro benchmark), info.Holder() is not free.

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 174281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174281&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        There's no reason to store info.Holder in a local variable. Calling
> 
> Just in case, would you check the performance in Bindings/first-child.html ? In the DOM binding benchmark suite in v8-internal (which is a super micro benchmark), info.Holder() is not free.

Sure, I'm happy to check it.

In this case, when we call info.Holder() a second time, we're actually going to call an out-of-line function that allocates a bunch of memory, etc, etc.  I probably should have said that it's free compared to those operations.  :)

[Adam Barth]

I looked at Bindings/first-child.html.  That doesn't actually execute the instruction that changed (since it always finds the wrapper).

[Adam Barth]

Created attachment 174318 [details]
Patch for landing

[Kentaro Hara]

Comment on attachment 174318 [details]
Patch for landing

Ah, makes sense. Your change just changes a slow path. Thanks.

[WebKit Review Bot]

Comment on attachment 174318 [details]
Patch for landing

Clearing flags on attachment: 174318

Committed r134729: <http://trac.webkit.org/changeset/134729>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

Updated run-bindings-tests results in r134738.