Bug 102297 - [V8] We can shave one instruction off toV8Fast
Summary: [V8] We can shave one instruction off toV8Fast
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-11-14 16:19 PST by Adam Barth
Modified: 2012-11-14 22:11 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.79 KB, patch)
2012-11-14 16:21 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (2.29 KB, patch)
2012-11-14 19:01 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-11-14 16:19:41 PST
[V8] We can shave one instruction off toV8Fast
Comment 1 Adam Barth 2012-11-14 16:21:16 PST
Created attachment 174281 [details]
Patch
Comment 2 Kentaro Hara 2012-11-14 16:24:44 PST
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.
Comment 3 Adam Barth 2012-11-14 16:28:29 PST
(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.  :)
Comment 4 Adam Barth 2012-11-14 19:01:19 PST
I looked at Bindings/first-child.html.  That doesn't actually execute the instruction that changed (since it always finds the wrapper).
Comment 5 Adam Barth 2012-11-14 19:01:45 PST
Created attachment 174318 [details]
Patch for landing
Comment 6 Kentaro Hara 2012-11-14 19:02:29 PST
Comment on attachment 174318 [details]
Patch for landing

Ah, makes sense. Your change just changes a slow path. Thanks.
Comment 7 WebKit Review Bot 2012-11-14 19:41:54 PST
Comment on attachment 174318 [details]
Patch for landing

Clearing flags on attachment: 174318

Committed r134729: <http://trac.webkit.org/changeset/134729>
Comment 8 WebKit Review Bot 2012-11-14 19:41:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Kentaro Hara 2012-11-14 22:11:52 PST
Updated run-bindings-tests results in r134738.