Bug 102082 - [V8] More efficient wrapper dispatch
Summary: [V8] More efficient wrapper dispatch
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: 100851
  Show dependency treegraph
 
Reported: 2012-11-13 07:12 PST by Dan Carney
Modified: 2012-11-27 09:39 PST (History)
3 users (show)

See Also:


Attachments
Patch (64.51 KB, patch)
2012-11-13 07:20 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch for landing (65.36 KB, patch)
2012-11-13 11:05 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 Dan Carney 2012-11-13 07:12:03 PST
[V8] More efficient wrapper dispatch
Comment 1 Dan Carney 2012-11-13 07:20:28 PST
Created attachment 173888 [details]
Patch
Comment 2 Adam Barth 2012-11-13 08:19:35 PST
Comment on attachment 173888 [details]
Patch

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

This looks great.  I'm not 100% sure about the names.  Here's a recommendation:

wrapSlow -> createWrapper
dispatchWrap -> wrap
dispatchWrapCustom -> wrap

When you have a CustomToJSObject, can't we just skip generating the implementation of wrap (aka dispatchWrap)?  I don't see what having the thunk buys us.  In any case, feel free to land the patch with the current names.  We can iterate from there if needed.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:498
> +    if ($interfaceName eq "HTMLElement") {
> +        push(@headerContent, <<END);
> +    friend v8::Handle<v8::Object> createV8HTMLWrapper(HTMLElement*, v8::Handle<v8::Object> creationContext, v8::Isolate*);
> +END
> +    } elsif ($interfaceName eq "SVGElement") {
> +        push(@headerContent, <<END);
> +    friend v8::Handle<v8::Object> createV8SVGWrapper(SVGElement* element, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate);

Why does the HTML version have named parameters but the SVG version does not?  We should probably make this consistent.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:519
>      static v8::Handle<v8::Object> wrapSlow(${wrapSlowArgumentType}, v8::Handle<v8::Object> creationContext, v8::Isolate*);

I was thinking of renaming wrapSlow to createWrapper (assuming it no longer checks the cache).

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:553
> +    if (UNLIKELY(!impl))
> +        return v8::Handle<v8::Object>();

Can this ever actually happen?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3425
>  v8::Handle<v8::Object> ${className}::wrapSlow(${wrapSlowArgumentType} impl, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate)

Yeah, this always creates a wrapper now, right?  We should name it createWrapper.  (We can do that in a separate patch if you like.)

> Source/WebCore/bindings/scripts/IDLAttributes.txt:122
> +V8NoWrapperCache=*

I think you want this to be just "V8NoWrapperCache" because the attribute doesn't take any arguments.
Comment 3 Adam Barth 2012-11-13 11:03:17 PST
/me is rebasing to top of tree.
Comment 4 Adam Barth 2012-11-13 11:05:05 PST
Created attachment 173929 [details]
Patch for landing
Comment 5 Adam Barth 2012-11-13 11:06:01 PST
I'm going to land this patch because I'd like to build upon it.
Comment 6 Adam Barth 2012-11-13 11:06:56 PST
Committed r134447: <http://trac.webkit.org/changeset/134447>
Comment 7 Csaba Osztrogonác 2012-11-13 11:28:24 PST
(In reply to comment #6)
> Committed r134447: <http://trac.webkit.org/changeset/134447>

It broke the Qt build :-/

[1;31mgenerated/JSSVGElementWrapperFactory.cpp:711: error: expected '}' at end of input[0m
Comment 8 Adam Barth 2012-11-13 11:30:24 PST
(In reply to comment #7)
> (In reply to comment #6)
> > Committed r134447: <http://trac.webkit.org/changeset/134447>
> 
> It broke the Qt build :-/
> 
> [1;31mgenerated/JSSVGElementWrapperFactory.cpp:711: error: expected '}' at end of input[0m

/me will fix.
Comment 9 Adam Barth 2012-11-13 11:43:46 PST
Attempted fix in http://trac.webkit.org/changeset/134457
Comment 10 Dan Carney 2012-11-13 12:45:36 PST
Comment on attachment 173888 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:498
>> +    friend v8::Handle<v8::Object> createV8SVGWrapper(SVGElement* element, v8::Handle<v8::Object> creationContext, v8::Isolate* isolate);
> 
> Why does the HTML version have named parameters but the SVG version does not?  We should probably make this consistent.

No good reason. Copy and paste issues. Will fix.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:519
>>      static v8::Handle<v8::Object> wrapSlow(${wrapSlowArgumentType}, v8::Handle<v8::Object> creationContext, v8::Isolate*);
> 
> I was thinking of renaming wrapSlow to createWrapper (assuming it no longer checks the cache).

Yeah, me too, I just didn't want to do all the renaming in one patch as I wanted the patch to focus on the change in wrap.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:553
>> +        return v8::Handle<v8::Object>();
> 
> Can this ever actually happen?

I don't know. I needed to copy the functionality of toV8, which has the check, but without returning v8::Null.

>> Source/WebCore/bindings/scripts/IDLAttributes.txt:122
>> +V8NoWrapperCache=*
> 
> I think you want this to be just "V8NoWrapperCache" because the attribute doesn't take any arguments.

okay. I wasn't sure what any of that meant
Comment 11 Dan Carney 2012-11-13 12:47:01 PST
(In reply to comment #5)
> I'm going to land this patch because I'd like to build upon it.

I'll try to respond to all the above comments in a fix up patch tomorrow. Did you change anything?
Comment 12 Dan Carney 2012-11-13 12:52:15 PST
> wrapSlow -> createWrapper
> dispatchWrap -> wrap
> dispatchWrapCustom -> wrap

okay, I just didn't want anyone using those functions, so I figured unusual names 

> When you have a CustomToJSObject, can't we just skip generating the implementation of wrap (aka dispatchWrap)?  I don't see what having the thunk buys us.  In any case, feel free to land the patch with the current names.  We can iterate from there if needed.

At first I wanted the asserts autogenerated, but I later added the same assert to wrapSlow. I can drop it and just go back to one non member function called wrap.
Comment 13 Adam Barth 2012-11-13 13:20:58 PST
> Did you change anything?

I just addressed the createV8HTMLWrapper / createV8SVGWrapper nit.
Comment 14 Csaba Osztrogonác 2012-11-13 22:28:01 PST
(In reply to comment #9)
> Attempted fix in http://trac.webkit.org/changeset/134457

Thanks for the quick fix.
Comment 15 Marshall Greenblatt 2012-11-27 09:39:13 PST
This change appears to have caused an infinite recursion problem in V8 when the <video> tag is used with MediaPlayer disabled. See https://bugs.webkit.org/show_bug.cgi?id=103431.