Bug 100851 – [V8] use toV8Fast in all relevant Node getters

• Component: New Bugs
• Status: RESOLVED FIXED

[Dan Carney]

[V8] use toV8Fast in all relevant getters

[Dan Carney]

Created attachment 171640 [details]
Patch

[Kentaro Hara]

Comment on attachment 171640 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:297
> +    die("IsDOMNodeType is out of date with respect to $interfaceName") if IsDOMNodeType($interfaceName) != IsNodeSubType($dataNode);

Why are both IsDOMNodeType() and IsNodeSubType() needed, given that they should return the same result?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1121
> +    } elsif (IsDOMNodeType($attrType) && IsDOMNodeType($interfaceName)) {
> +        AddToImplIncludes($attrType.".h");
>          push(@implContentDecls, "    return toV8Fast($result, info, imp);\n");

Some Node-type classes have custom implementation of toV8() in bindings/v8/custom/*. Wouldn't it cause a problem? I mean, if you call toV8Fast() here, you might end up failing to call custom toV8().

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-4031
> -    return 1 if $type eq 'HTMLElement';
> -    return 1 if $type eq 'HTMLUnknownElement';
> -    return 1 if $type eq 'HTMLFormElement';
> -    return 1 if $type eq 'HTMLTableCaptionElement';
> -    return 1 if $type eq 'HTMLTableSectionElement';

Isn't IsDOMNodeType() used by other places? I want to make sure that this change won't affect other places.

[Dan Carney]

Comment on attachment 171640 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:297
>> +    die("IsDOMNodeType is out of date with respect to $interfaceName") if IsDOMNodeType($interfaceName) != IsNodeSubType($dataNode);
> 
> Why are both IsDOMNodeType() and IsNodeSubType() needed, given that they should return the same result?

One is only useful for the currently executing dataNode. The other can check the DOMNode-iness of anything based on name.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1121
>>          push(@implContentDecls, "    return toV8Fast($result, info, imp);\n");
> 
> Some Node-type classes have custom implementation of toV8() in bindings/v8/custom/*. Wouldn't it cause a problem? I mean, if you call toV8Fast() here, you might end up failing to call custom toV8().

Yep.  Fix on its way.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:-4031
>> -    return 1 if $type eq 'HTMLTableSectionElement';
> 
> Isn't IsDOMNodeType() used by other places? I want to make sure that this change won't affect other places.

It's used in a few other places. I can check it out, but it's pretty poorly named if this update is not correct.

[Dan Carney]

Created attachment 171854 [details]
Patch

[Adam Barth]

Comment on attachment 171854 [details]
Patch

This looks great!

[Kentaro Hara]

Comment on attachment 171854 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:527
> +    my $has_empty_toV8 = 0;
> +    my $has_fast_toV8 = 0;

WebKit uses camelCase for variable names.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:604
> +    die("HasFastToV8 is out of sync with respect to interface $interfaceName [$has_fast_toV8]\n") if HasFastToV8($interfaceName) != $has_fast_toV8;
> +    die("HasEmptyToV8 is out of sync with respect to interface $interfaceName [$has_fast_toV8]\n") if HasEmptyToV8($interfaceName) != $has_empty_toV8;

Basically we don't want to introduce hard-coded class lists (i.e. HasFastToV8() and HasEmptyToV8()). See below comments.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4060
> +sub HasEmptyToV8
> +{
> +    my $type = shift;
> +
> +    return 1 if $type eq 'Element';
> +    return 1 if $type eq 'SVGRenderingIntent';
> +    return 1 if $type eq 'SVGUnitTypes';
> +    return 1 if $type eq 'SVGZoomAndPan';
> +
> +    return 0;
> +}

What is this?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4071
> +    return 0 if $type eq 'Document';
> +    return 0 if $type eq 'Element';
> +    return 0 if $type eq 'HTMLDocument';
> +    return 0 if $type eq 'HTMLElement';
> +    return 0 if $type eq 'SVGDocument';
> +    return 0 if $type eq 'SVGElement';

Wouldn't it be possible to rename toV8() in custom bindings to toV8Fast() and thus remove this hard-coded class list from CodeGeneratorV8.pm? (It would be better if we could make the implementation of custom toV8Fast() *fast*er, but let's do it in follow-up patches.)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4076
>  sub IsDOMNodeType

Can't we just replace IsDOMNodeType() with IsNodeSubType(), and thus kill IsDOMNodeType() ?

[Adam Barth]

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:545
> +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* node, const v8::AccessorInfo& info, Node* holder)

It's unfortunate that we now have two copies of this function definition.  Maybe we could make a helper "generate" function to share the code?

[Kentaro Hara]

(In reply to comment #7)
> View in context: https://bugs.webkit.org/attachment.cgi?id=171854&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:545
> > +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* node, const v8::AccessorInfo& info, Node* holder)
> 
> It's unfortunate that we now have two copies of this function definition.  Maybe we could make a helper "generate" function to share the code?

Or can we just generate nothing so that the virtualized Node::toV8Fast() is called ?


dcarney: Sorry for setting my r? by mistake... please reset it:)

[Adam Barth]

Comment on attachment 171854 [details]
Patch

Please address haraken's comments before landing.  It might be worth posting another iteration of the patch for review.

[Kentaro Hara]

Comment on attachment 171854 [details]
Patch

I discussed with dcarney a bit more offline. To clean up confusing toV8(), toV8Fast(), toV8Slow() and custom toV8(), we decided to land the patch step by step. For now marking r-.

[Dan Carney]

Created attachment 174099 [details]
Patch

[Dan Carney]

(In reply to comment #11)
> Created an attachment (id=174099) [details]
> Patch

As per Kentaro's request, we now have toV8Fast on all Node subclasses which dispatches correctly in all cases (in some cases to regular toV8).  toV8Slow was removed in another patch.

[Kentaro Hara]

Comment on attachment 174099 [details]
Patch

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

Looks good to me as an incremental step to implement toV8Fast() and kill toV8Slow().

Just to clarify, would you explain what code will be executed for HTMLDivElement::toV8Fast() by this change?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:606
> +        V8DOMWindowShell* context = V8DOMWindowShell::getEntered();
> +        ASSERT(context);
> +        v8::Handle<v8::Object> wrapper = context->world()->isolatedWorldDOMDataStore()->get(impl);

Consider a change from abarth (https://bugs.webkit.org/show_bug.cgi?id=102156).

[Kentaro Hara]

(In reply to comment #13)
> (From update of attachment 174099 [details])
> Looks good to me as an incremental step to implement toV8Fast() and kill toV8Slow().
> 
> Just to clarify, would you explain what code will be executed for HTMLDivElement::toV8Fast() by this change?

Maybe you need to update run-bindings-tests results.

[Dan Carney]

Created attachment 174117 [details]
Patch

[Dan Carney]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 174099 [details] [details])
> > Looks good to me as an incremental step to implement toV8Fast() and kill toV8Slow().
> > 
> > Just to clarify, would you explain what code will be executed for HTMLDivElement::toV8Fast() by this change?
> 
> Maybe you need to update run-bindings-tests results.

fixed

[Dan Carney]

(In reply to comment #13)
> (From update of attachment 174099 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174099&action=review
> 
> Looks good to me as an incremental step to implement toV8Fast() and kill toV8Slow().
> 
> Just to clarify, would you explain what code will be executed for HTMLDivElement::toV8Fast() by this change?

it will call toV8Fast(HTMLDivElement*... which will perform the fast check and on cache miss, dispatch the wrapping call through dispatchWrap(HTMLDivElement*... instead of dispatchWrap(Node*..., which would eventually call dispatchWrap(HTMLDivElement*...  anyway

[Kentaro Hara]

Comment on attachment 174117 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 174117 [details]
Patch

Clearing flags on attachment: 174117

Committed r134588: <http://trac.webkit.org/changeset/134588>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.