Bug 102297

Summary: [V8] We can shave one instruction off toV8Fast
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: dcarney, eric, haraken, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Adam Barth
Reported 2012-11-14 16:19:41 PST
[V8] We can shave one instruction off toV8Fast
Attachments
Patch (3.79 KB, patch)
2012-11-14 16:21 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
Patch for landing (2.29 KB, patch)
2012-11-14 19:01 PST, Adam Barth 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Adam Barth
Comment 1 2012-11-14 16:21:16 PST
Created attachment 174281 [details] Patch
Kentaro Hara
Comment 2 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.
Adam Barth
Comment 3 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. :)
Adam Barth
Comment 4 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).
Adam Barth
Comment 5 2012-11-14 19:01:45 PST
Created attachment 174318 [details] Patch for landing
Kentaro Hara
Comment 6 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.
WebKit Review Bot
Comment 7 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>
WebKit Review Bot
Comment 8 2012-11-14 19:41:58 PST
All reviewed patches have been landed. Closing bug.
Kentaro Hara
Comment 9 2012-11-14 22:11:52 PST
Updated run-bindings-tests results in r134738.
Note You need to log in before you can comment on or make changes to this bug.