Bug 98567

Summary: [V8] toV8(Node*, ...) does more work than needed (6% faster on dom-traverse)
Product: WebKit Reporter: Adam Barth <abarth>
Component: New BugsAssignee: Adam Barth <abarth>
Status: RESOLVED FIXED    
Severity: Normal CC: haraken, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
work in progress
none
dom-traverse results (6% better; first 3 runs are with patch, last 3 are without)
none
Patch
none
Patch
none
Patch for landing none

Description Adam Barth 2012-10-05 15:42:03 PDT 
[V8] toV8(Node*, ...) does more work than needed
Comment 1 Adam Barth 2012-10-05 15:42:21 PDT 
Created attachment 167399 [details]
work in progress
Comment 2 Adam Barth 2012-10-05 15:52:55 PDT 
Created attachment 167404 [details]
dom-traverse results (6% better; first 3 runs are with patch, last 3 are without)
Comment 3 Adam Barth 2012-10-05 16:08:38 PDT 
Created attachment 167405 [details]
Patch
Comment 4 Adam Barth 2012-10-05 16:09:45 PDT 
Created attachment 167406 [details]
Patch
Comment 5 Kentaro Hara 2012-10-05 17:25:37 PDT 
Comment on attachment 167406 [details]
Patch

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

Awesome!

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:534
> +    if (holder->wrapper() && *holder->wrapper() == holderWrapper && node->wrapper())

This looks a bit hacky. Shall we add a comment on this?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:535
> +        return *node->wrapper();

Maybe you can now remove the logic to check node->wrapper() from V8DOMWrapper::getCachedWrapper()? (You can do it in a follow-up patch.)
Comment 6 Adam Barth 2012-10-05 18:13:21 PDT 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:534
> > +    if (holder->wrapper() && *holder->wrapper() == holderWrapper && node->wrapper())
> 
> This looks a bit hacky. Shall we add a comment on this?

Add a comment is a good idea.  It's a subtle trick.

> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:535
> > +        return *node->wrapper();
> 
> Maybe you can now remove the logic to check node->wrapper() from V8DOMWrapper::getCachedWrapper()? (You can do it in a follow-up patch.)

We can once we move all callers over to toV8Fast.
Comment 7 Adam Barth 2012-10-05 18:34:44 PDT 
Created attachment 167433 [details]
Patch for landing
Comment 8 WebKit Review Bot 2012-10-05 19:08:42 PDT 
Comment on attachment 167433 [details]
Patch for landing

Clearing flags on attachment: 167433

Committed r130574: <http://trac.webkit.org/changeset/130574>
Comment 9 WebKit Review Bot 2012-10-05 19:08:45 PDT 
All reviewed patches have been landed.  Closing bug.