Bug 32563 – [V8] Generate toV8 conversion helpers, a la JSC bindings.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Dimitri Glazkov (Google)]

This is going to be sooo pretty.

[Nate Chapin]

Created attachment 46481 [details]
Generate toNative() converter (toV8() will be done in a later patch)

[Dimitri Glazkov (Google)]

Comment on attachment 46481 [details]
Generate toNative() converter (toV8() will be done in a later patch)

Going native on V8 bindings, eh? :D

[Nate Chapin]

Comment on attachment 46481 [details]
Generate toNative() converter (toV8() will be done in a later patch)

http://trac.webkit.org/changeset/53204

[Nate Chapin]

Created attachment 47228 [details]
First attempt at generating toV8()

This very well may not be the right answer, but it's one way to generate toV8() converters.  This patch is primarily just generating the converters and making the generated bindings use them.  The custom bindings still use the old converters.

The biggest sticking point of this idea is that it moves a lot of the special cases that currently live in V8DOMWrapper.cpp into CodeGeneratorV8.pm.  I could see a case against that :)

Input appreciated.

[WebKit Review Bot]

Attachment 47228 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/206131

[Nate Chapin]

Comment on attachment 47228 [details]
First attempt at generating toV8()

Ewww.  Obsoleting while I look into what happened here.

[Nate Chapin]

Created attachment 47352 [details]
Attempt #1.1 at generating toV8()

I think (and hope) that I've accounted for the differences between compiler nits now.

[Nate Chapin]

Comment on attachment 47352 [details]
Attempt #1.1 at generating toV8()

After discussion with dglazkov, we agree that this is almost certainly not the right answer, so obsoleting this patch.

[Nate Chapin]

Created attachment 47550 [details]
toV8() Attempt #2.0

[WebKit Review Bot]

Attachment 47550 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/216152

[Nate Chapin]

Comment on attachment 47550 [details]
toV8() Attempt #2.0

I appear to have forgotten an #if ENABLED guard.

[Nate Chapin]

Created attachment 47576 [details]
toV8() Attempt #3.0

This is my current thought for handling the casting problems for HTMLElement and SVGElement. I just wanted to get it some eyeballs, since I'll be out of office tomorrow.  It changes dom/make_names.pl to support generating wrapping code for V8 (it currently is hard-coded for JSC) and therefore uses a nearly identical technique to what JSC does.

[anton muhin]

(In reply to comment #12)
> Created an attachment (id=47576) [details]
> toV8() Attempt #3.0
> 
> This is my current thought for handling the casting problems for HTMLElement
> and SVGElement. I just wanted to get it some eyeballs, since I'll be out of
> office tomorrow.  It changes dom/make_names.pl to support generating wrapping
> code for V8 (it currently is hard-coded for JSC) and therefore uses a nearly
> identical technique to what JSC does.

Several concerns.

1) object wrapping is very performance critical, so, please, verify that your patch doesn't affect performance;  and it might be better to have separation of create new vs. create on higher level (not threading the flag);

2) former V8DOMWrapper::convertNode... is pretty complicated code and lifting it as a whole into code generator looks problematic for me.  For example, any experiment would lead to rebuild all and compile-run cycle would be very long. Overall I'd prefer to keep generated non-trivial code at the minimum.  Is it possible to keep most of conversion logic in plain C++ and only invoke necessary bits from codegen?  For example:

 1983   if (V8IsolatedContext::getEntered())
 1984     proxy = 0;
 1985   else if (!proxy)
 1986     proxy = V8Proxy::retrieve();
 1987 
 1988   if (proxy)
 1989     wrapper = proxy->windowShell()->createWrapperFromCache(${wrapperType});
 1990   else
 1991     wrapper = SafeAllocation::newInstance(V8DOMWrapper::getTemplate(${wrapperType})->GetFunction());
 1992   if (!wrapper.IsEmpty())
 1993     V8DOMWrapper::setDOMWrapper(wrapper, V8ClassIndex::ToInt(${wrapperType}), impl);
 1994 END
 1995 

This looks like a perfect inline function on its own.

3)

 sub HasCustomToV8Implementation {
 2043     $dataNode = shift;
 2044     $interfaceName = shift;
 2045     
 2046     # We generate a custom converter (but JSC doesn't) for the following:
 2047     return 1 if $interfaceName eq "BarInfo";
 2048     return 1 if $interfaceName eq "CSSStyleSheet";
 2049     return 1 if $interfaceName eq "CanvasPixelArray";
 2050     return 1 if $interfaceName eq "Console";
 2051     return 1 if $interfaceName eq "DOMSelection";
 2052     return 1 if $interfaceName eq "DOMWindow";
 2053     return 1 if $interfaceName eq "Element";
 2054     return 1 if $interfaceName eq "Location";
 2055     return 1 if $interfaceName eq "HTMLDocument";
 2056     return 1 if $interfaceName eq "HTMLElement";
 2057     return 1 if $interfaceName eq "History";
 2058     return 1 if $interfaceName eq "NamedNodeMap";
 2059     return 1 if $interfaceName eq "Navigator";
 2060     return 1 if $interfaceName eq "SVGDocument";
 2061     return 1 if $interfaceName eq "SVGElement";
 2062     return 1 if $interfaceName eq "Screen";
 2063     
 2064     # We don't generate a custom converter (but JSC does) for the following:
 2065     return 0 if $interfaceName eq "AbstractWorker";
 2066     return 0 if $interfaceName eq "CanvasRenderingContext";
 2067     return 0 if $interfaceName eq "HTMLCollection";
 2068     return 0 if $interfaceName eq "ImageData";
 2069     return 0 if $interfaceName eq "SVGElementInstance";
 2070 
 2071     # For everything else, do what JSC does.
 2072     return $dataNode->extendedAttributes->{"CustomToJS"};
 2073 }
 2074 

Shouldn't that be an attribute in IDL?

Same question for

 2088 sub IsActiveDomType
 2089 {
 2090     my $type = shift;
 2091     return 1 if $type eq "MessagePort";
 2092     return 1 if $type eq "XMLHttpRequest";
 2093     return 1 if $type eq "WebSocket";
 2094     return 1 if $type eq "Worker";
 2095     return 1 if $type eq "SharedWorker";
 2096     return 0;
 2097 }

4) This change from double to float, is it fine?  Could it be done in a separate CL?

5)

 1942   //if (!impl)
 1943   //  return v8::Null();

should be probably removed.

And thanks a lot for keeping me in the loop.

[Dimitri Glazkov (Google)]

Comment on attachment 47576 [details]
toV8() Attempt #3.0

Overall, I think the 3.0 is definitely better than the 1.0, but Anton has good comments and I'll go with him.

[Nate Chapin]

(In reply to comment #13)
> 1) object wrapping is very performance critical, so, please, verify that your
> patch doesn't affect performance;  and it might be better to have separation of
> create new vs. create on higher level (not threading the flag);
> 

Will do on the perf tests.  For the separation of new vs. existing objects, I don't think it's feasible.  Typically, we start with a return type of something like "Element" for items that are labelled "ReturnsNew" in an IDL.  So, e.g., toV8(Element*) is calling toV8(HTMLElement*) is calling toV8(HTMLSomeOtherKindOfElement*), and I think it's going to make the code even uglier than it is currently to have a separate "create new object" code path.

> 2) former V8DOMWrapper::convertNode... is pretty complicated code and lifting
> it as a whole into code generator looks problematic for me.  For example, any
> experiment would lead to rebuild all and compile-run cycle would be very long.
> Overall I'd prefer to keep generated non-trivial code at the minimum.  Is it
> possible to keep most of conversion logic in plain C++ and only invoke
> necessary bits from codegen?  For example:
> 
>  1983   if (V8IsolatedContext::getEntered())
>  1984     proxy = 0;
>  1985   else if (!proxy)
>  1986     proxy = V8Proxy::retrieve();
>  1987 
>  1988   if (proxy)
>  1989     wrapper =
> proxy->windowShell()->createWrapperFromCache(${wrapperType});
>  1990   else
>  1991     wrapper =
> SafeAllocation::newInstance(V8DOMWrapper::getTemplate(${wrapperType})->GetFunction());
>  1992   if (!wrapper.IsEmpty())
>  1993     V8DOMWrapper::setDOMWrapper(wrapper,
> V8ClassIndex::ToInt(${wrapperType}), impl);
>  1994 END
>  1995 
> 
> This looks like a perfect inline function on its own.

Yeah, I got experimenting with generating that.  I think inlining is the right answer after all.  That block is precisely what V8DOMWrapper::instantiateV8Object() does.

> 
> 3)
> 
>  sub HasCustomToV8Implementation {
>  2043     $dataNode = shift;
>  2044     $interfaceName = shift;
>  2045     
>  2046     # We generate a custom converter (but JSC doesn't) for the following:
>  2047     return 1 if $interfaceName eq "BarInfo";
>  2048     return 1 if $interfaceName eq "CSSStyleSheet";
>  2049     return 1 if $interfaceName eq "CanvasPixelArray";
>  2050     return 1 if $interfaceName eq "Console";
>  2051     return 1 if $interfaceName eq "DOMSelection";
>  2052     return 1 if $interfaceName eq "DOMWindow";
>  2053     return 1 if $interfaceName eq "Element";
>  2054     return 1 if $interfaceName eq "Location";
>  2055     return 1 if $interfaceName eq "HTMLDocument";
>  2056     return 1 if $interfaceName eq "HTMLElement";
>  2057     return 1 if $interfaceName eq "History";
>  2058     return 1 if $interfaceName eq "NamedNodeMap";
>  2059     return 1 if $interfaceName eq "Navigator";
>  2060     return 1 if $interfaceName eq "SVGDocument";
>  2061     return 1 if $interfaceName eq "SVGElement";
>  2062     return 1 if $interfaceName eq "Screen";
>  2063     
>  2064     # We don't generate a custom converter (but JSC does) for the
> following:
>  2065     return 0 if $interfaceName eq "AbstractWorker";
>  2066     return 0 if $interfaceName eq "CanvasRenderingContext";
>  2067     return 0 if $interfaceName eq "HTMLCollection";
>  2068     return 0 if $interfaceName eq "ImageData";
>  2069     return 0 if $interfaceName eq "SVGElementInstance";
>  2070 
>  2071     # For everything else, do what JSC does.
>  2072     return $dataNode->extendedAttributes->{"CustomToJS"};
>  2073 }
>  2074 
> 
> Shouldn't that be an attribute in IDL?
> 
> Same question for
> 
>  2088 sub IsActiveDomType
>  2089 {
>  2090     my $type = shift;
>  2091     return 1 if $type eq "MessagePort";
>  2092     return 1 if $type eq "XMLHttpRequest";
>  2093     return 1 if $type eq "WebSocket";
>  2094     return 1 if $type eq "Worker";
>  2095     return 1 if $type eq "SharedWorker";
>  2096     return 0;
>  2097 }

Adding FIXMEs for these.  That may be the right long-term solution.

> 
> 4) This change from double to float, is it fine?  Could it be done in a
> separate CL?

We're pretty loose with double<->float conversion in here, and my changes caused some of those to become compile errors, so I figured standardizing made sense.  All the layout tests still pass, but if there's something else you would like me to do to verify that this change is fine, let me know.

> 
> 5)
> 
>  1942   //if (!impl)
>  1943   //  return v8::Null();
> 
> should be probably removed.

Doh.

> 
> And thanks a lot for keeping me in the loop.

Next patch will be uploaded momentarily.

[Nate Chapin]

Created attachment 47721 [details]
Attempt #3.1

[anton muhin]

(In reply to comment #15)
> (In reply to comment #13)
> > 1) object wrapping is very performance critical, so, please, verify that your
> > patch doesn't affect performance;  and it might be better to have separation of
> > create new vs. create on higher level (not threading the flag);
> > 
> 
> Will do on the perf tests.  For the separation of new vs. existing objects, I
> don't think it's feasible.  Typically, we start with a return type of something
> like "Element" for items that are labelled "ReturnsNew" in an IDL.  So, e.g.,
> toV8(Element*) is calling toV8(HTMLElement*) is calling
> toV8(HTMLSomeOtherKindOfElement*), and I think it's going to make the code even
> uglier than it is currently to have a separate "create new object" code path.
>

From my experience, this separation is really important.  I don't quite understand why it cannot be implemented---after all the main difference is absence of check if object is already wrapped or not.  Maybe I am missing some context here.

> > 2) former V8DOMWrapper::convertNode... is pretty complicated code and lifting
> > it as a whole into code generator looks problematic for me.  For example, any
> > experiment would lead to rebuild all and compile-run cycle would be very long.
> > Overall I'd prefer to keep generated non-trivial code at the minimum.  Is it
> > possible to keep most of conversion logic in plain C++ and only invoke
> > necessary bits from codegen?  For example:
> > 
> >  1983   if (V8IsolatedContext::getEntered())
> >  1984     proxy = 0;
> >  1985   else if (!proxy)
> >  1986     proxy = V8Proxy::retrieve();
> >  1987 
> >  1988   if (proxy)
> >  1989     wrapper =
> > proxy->windowShell()->createWrapperFromCache(${wrapperType});
> >  1990   else
> >  1991     wrapper =
> > SafeAllocation::newInstance(V8DOMWrapper::getTemplate(${wrapperType})->GetFunction());
> >  1992   if (!wrapper.IsEmpty())
> >  1993     V8DOMWrapper::setDOMWrapper(wrapper,
> > V8ClassIndex::ToInt(${wrapperType}), impl);
> >  1994 END
> >  1995 
> > 
> > This looks like a perfect inline function on its own.
> 
> Yeah, I got experimenting with generating that.  I think inlining is the right
> answer after all.  That block is precisely what
> V8DOMWrapper::instantiateV8Object() does.
> 
> > 
> > 3)
> > 
> >  sub HasCustomToV8Implementation {
> >  2043     $dataNode = shift;
> >  2044     $interfaceName = shift;
> >  2045     
> >  2046     # We generate a custom converter (but JSC doesn't) for the following:
> >  2047     return 1 if $interfaceName eq "BarInfo";
> >  2048     return 1 if $interfaceName eq "CSSStyleSheet";
> >  2049     return 1 if $interfaceName eq "CanvasPixelArray";
> >  2050     return 1 if $interfaceName eq "Console";
> >  2051     return 1 if $interfaceName eq "DOMSelection";
> >  2052     return 1 if $interfaceName eq "DOMWindow";
> >  2053     return 1 if $interfaceName eq "Element";
> >  2054     return 1 if $interfaceName eq "Location";
> >  2055     return 1 if $interfaceName eq "HTMLDocument";
> >  2056     return 1 if $interfaceName eq "HTMLElement";
> >  2057     return 1 if $interfaceName eq "History";
> >  2058     return 1 if $interfaceName eq "NamedNodeMap";
> >  2059     return 1 if $interfaceName eq "Navigator";
> >  2060     return 1 if $interfaceName eq "SVGDocument";
> >  2061     return 1 if $interfaceName eq "SVGElement";
> >  2062     return 1 if $interfaceName eq "Screen";
> >  2063     
> >  2064     # We don't generate a custom converter (but JSC does) for the
> > following:
> >  2065     return 0 if $interfaceName eq "AbstractWorker";
> >  2066     return 0 if $interfaceName eq "CanvasRenderingContext";
> >  2067     return 0 if $interfaceName eq "HTMLCollection";
> >  2068     return 0 if $interfaceName eq "ImageData";
> >  2069     return 0 if $interfaceName eq "SVGElementInstance";
> >  2070 
> >  2071     # For everything else, do what JSC does.
> >  2072     return $dataNode->extendedAttributes->{"CustomToJS"};
> >  2073 }
> >  2074 
> > 
> > Shouldn't that be an attribute in IDL?
> > 
> > Same question for
> > 
> >  2088 sub IsActiveDomType
> >  2089 {
> >  2090     my $type = shift;
> >  2091     return 1 if $type eq "MessagePort";
> >  2092     return 1 if $type eq "XMLHttpRequest";
> >  2093     return 1 if $type eq "WebSocket";
> >  2094     return 1 if $type eq "Worker";
> >  2095     return 1 if $type eq "SharedWorker";
> >  2096     return 0;
> >  2097 }
> 
> Adding FIXMEs for these.  That may be the right long-term solution.
> 
> > 
> > 4) This change from double to float, is it fine?  Could it be done in a
> > separate CL?
> 
> We're pretty loose with double<->float conversion in here, and my changes
> caused some of those to become compile errors, so I figured standardizing made
> sense.  All the layout tests still pass, but if there's something else you
> would like me to do to verify that this change is fine, let me know.

Makes sense to me, thanks for clarifications.  I don't know how you feel about it, but this unification might be better done with a separate change, before this change goes in---it's already complicated enough.  But that's completely up to you.

New batch:

1)
 842             } else {
 843                 print F <<END

tabs got into your files---beware!

2) current state of toV8Helper might have performance implications---you could lookup a map to put object into twice---first time to get a map to check if wrapper is present and second time to add a wrapper into the map.  You might be better storing this map into a local variable and reuse it later (cf. to the current implementation.)

Note as well, that recently another fast check was added for the case of the Node---we check a pointer of the object itself (please let me know if you need more details.)

3)

112          ASSERT(V8DOMWrapper::domWrapperType(info.Holder()) == V8ClassIndex::NODE);

Why this is removed?

4)

 196     default: break; // XPATH_NAMESPACE_NODE

What does this comment mean?

5)
79      ASSERT(V8DOMWrapper::domWrapperType(info.Holder()) == V8ClassIndex::NODE);

Ditto, why removed?

6) this

 61     v8::Handle<v8::Object> wrapper = getDOMObjectMap().get(impl);
 62     if (wrapper.IsEmpty()) {
 63         wrapper = V8Console::toV8Helper(impl);
 64         if (!wrapper.IsEmpty())
 65             V8DOMWrapper::setHiddenWindowReference(impl->frame(), V8DOMWindow::consoleIndex, wrapper);
 66     }

 and owner handling seems a pretty common pattern.  Maybe generalize it in this CL?  Or you'd better postpone it for some further time?

7)

 90     bindings/v8/custom/V8BarInfoCustom.cpp \
 91     bindings/v8/custom/V8CSSRuleCustom.cpp \

here and below, why additional indent?

[Nate Chapin]

Comment on attachment 47721 [details]
Attempt #3.1

Obsoleting this patch, I need to make a couple more minor changes.

[Nate Chapin]

Created attachment 47864 [details]
Attempt #3.2

The two big changes in this version are:
1. Renamed toV8Helper() to wrap()
2. Integrated http://trac.webkit.org/changeset/53944 into the generated node wrapping functions.

[Dimitri Glazkov (Google)]

Comment on attachment 47864 [details]
Attempt #3.2

Yay! r=me, after fixing:

> +    switch (impl->pathSegType()) {
> +    case SVGPathSeg::PATHSEG_CLOSEPATH:                    return toV8(static_cast<SVGPathSegClosePath*>(impl));

I know you copied this over, but this needs to be in WebKit style, with line breaks/indents.

> Index: WebCore/Android.v8bindings.mk
> ===================================================================
> --- WebCore/Android.v8bindings.mk	(revision 54136)
> +++ WebCore/Android.v8bindings.mk	(working copy)
> @@ -87,8 +87,13 @@
>  	\
>  	bindings/v8/custom/V8AbstractWorkerCustom.cpp \
>  	bindings/v8/custom/V8AttrCustom.cpp \
> +    bindings/v8/custom/V8BarInfoCustom.cpp \
> +    bindings/v8/custom/V8CSSRuleCustom.cpp \
>  	bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp \
> +    bindings/v8/custom/V8CSSStyleSheetCustom.cpp \
> +    bindings/v8/custom/V8CSSValueCustom.cpp \
>  	bindings/v8/custom/V8CanvasRenderingContext2DCustom.cpp \
> +    bindings/v8/custom/V8CanvasPixelArrayCustom.cpp \

Tabs in this file?

[Nate Chapin]

Comment on attachment 47864 [details]
Attempt #3.2

Landed in http://trac.webkit.org/changeset/54150 and http://trac.webkit.org/changeset/54153. Leaving bug open because we still need to switch the custom bindings over to using toV8().

[Nate Chapin]

Created attachment 47974 [details]
Switch custom bindings over to toV8()

Will run perf tests locally momentarily.

[Dimitri Glazkov (Google)]

Comment on attachment 47974 [details]
Switch custom bindings over to toV8()

Hurray! I thereby deem this the coolest patch eva.

[Nate Chapin]

http://trac.webkit.org/changeset/54259

[Shinichiro Hamaji]

Committed r54278: <http://trac.webkit.org/changeset/54278>

[Shinichiro Hamaji]

I reverted this change as it seems to break chromium's unit tests. This is an example of the failures:

http://build.chromium.org/buildbot/try-server/buildstatus?builder=win&number=17123

[Nate Chapin]

Created attachment 48067 [details]
Now without Chromium test failures!

Two changes:
1. Added some code to CodeGeneratorV8.pm to use WorkerContextExecutionProxy when necessary to actually do the wrapping instead of V8DOMWrapper.
2. Fixed a bug where I was incorrectly returning early in allowPopUp() in V8DOMWindowCustom.cpp.

[Dimitri Glazkov (Google)]

Comment on attachment 48067 [details]
Now without Chromium test failures!

ok. Make sure to check w/ukai about WebSockets + Workers.

[Nate Chapin]

Landed (again): http://trac.webkit.org/changeset/54305

[Shinichiro Hamaji]

(In reply to comment #29)
> Landed (again): http://trac.webkit.org/changeset/54305

It seems the roll succeeded this time. Thanks for the fix!