Bug 102082 – [V8] More efficient wrapper dispatch

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

[Dan Carney]

[V8] More efficient wrapper dispatch

[Dan Carney]

Created attachment 173888 [details]
Patch

[Adam Barth]

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.

[Adam Barth]

/me is rebasing to top of tree.

[Adam Barth]

Created attachment 173929 [details]
Patch for landing

[Adam Barth]

I'm going to land this patch because I'd like to build upon it.

[Adam Barth]

Committed r134447: <http://trac.webkit.org/changeset/134447>

[Csaba Osztrogonác]

(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

[Adam Barth]

(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.

[Adam Barth]

Attempted fix in http://trac.webkit.org/changeset/134457

[Dan Carney]

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

[Dan Carney]

(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?

[Dan Carney]

> 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.

[Adam Barth]

> Did you change anything?

I just addressed the createV8HTMLWrapper / createV8SVGWrapper nit.

[Csaba Osztrogonác]

(In reply to comment #9)
> Attempted fix in http://trac.webkit.org/changeset/134457

Thanks for the quick fix.

[Marshall Greenblatt]

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.