Bug 149952 – Binding generator should use templated JSXXConstructor

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[youenn fablet]

Using templated JSXXConstructor would reduce the size of the binding generator code.

[youenn fablet]

Created attachment 262773 [details]
Patch

[WebKit Commit Bot]

Attachment 262773 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/JSDOMConstructor.h:46:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMConstructor.h:77:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/JSDOMConstructor.h:116:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 3 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Looks like a good idea.

[youenn fablet]

Comment on attachment 262773 [details]
Patch

Marking patch as r?

I am not sure of the actual value of JSDOMNamedConstructor and DOMConstructorWithDocument.
Maybe we can try removing them later on.

[Darin Adler]

Comment on attachment 262773 [details]
Patch

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

Generally I find these class templates easier to read if we put the function definitions all outside the class template definition. Even though the total file size is bigger. Inlining can be controlled with the inline keyword rather than by putting the functions to be inlined into the class definition.

I think I am having a little trouble seeing the big picture here and understanding the strategy. I don’t fully understand the patch. I do think it’s OK to use templates more so we can generate less code from the code generation script, as long as we don’t cause an explosion in compile time or binary size by doing that. But there is something peculiar about these new templates. It’s really unclear what part of the template goes into the header and what part is designed be specialized. Might even be worth commenting on that in the class template definition.

Also note that some of my questions about references vs. pointers are just that: questions. Don’t assume that we could do all of those, since I’m sure some are functions that are called by template code or by macros and so have to have the right argument types; just wanted you to consider if they could change.

> Source/WebCore/ChangeLog:18
> +        A further patch should remove DOMConstructorWithDocument and DOMConstructorJSBuiltinObject. 

Typically it’s helpful to keep class template as small as possible. It can be useful to put code in non-template base classes, even if the only use of those classes is in a single class template. Thus I am not 100% sure it will be good to remove these.

> Source/WebCore/bindings/js/JSDOMConstructor.h:27
> +template <typename JSClass> class JSDOMConstructorNotConstructable : public DOMConstructorObject {

I don’t understand why this is a template. Nothing in this class template seems to depend on the JSClass argument. Even if we do have some reason to need a class template, is everything in this template needed? It’s often valuable to put as much as possible into a base class. I suppose that’s the role of DOMConstructorObject here? Maybe the main point of using a template here is that we will explicitly specialize one of the function or data members? 

Tiny nit: In new code we are not putting a space between "template" and the following <>.

> Source/WebCore/bindings/js/JSDOMConstructor.h:31
> +    static JSDOMConstructorNotConstructable* create(JSC::VM& vm, JSC::Structure* structure, JSDOMGlobalObject& globalObject)

Can this take a reference to the structure instead of a pointer?

Can this return a reference to the new object instead of a pointer?

> Source/WebCore/bindings/js/JSDOMConstructor.h:38
> +    static JSC::Structure* createStructure(JSC::VM& vm, JSC::JSGlobalObject& globalObject, JSC::JSValue prototype)

Can this return a reference to the structure instead of a pointer?

> Source/WebCore/bindings/js/JSDOMConstructor.h:46
> +    JSDOMConstructorNotConstructable(JSC::Structure* structure, JSDOMGlobalObject& globalObject) : Base(structure, globalObject) { }

Can this take a reference to the structure instead of a pointer?

> Source/WebCore/bindings/js/JSDOMConstructor.h:48
> +    void initializeProperties(JSC::VM&, JSDOMGlobalObject&) { }

I don’t understand the value of defining an inline private empty function and also writing code that calls it. We should omit both the function and calls to it.

> Source/WebCore/bindings/js/JSDOMConstructor.h:51
> +template<typename JSClass> void JSDOMConstructorNotConstructable<JSClass>::finishCreation(JSC::VM& vm, JSDOMGlobalObject& globalObject)

Seems this should probably be marked inline, but not sure who calls this.

> Source/WebCore/bindings/js/JSDOMConstructor.h:58
> +template <typename JSClass> class JSDOMConstructor : public DOMConstructorObject {

Is there a way to save code here? This seems really similar to JSDOMConstructorNotConstructable but lots of code seems to be repeated.

Tiny nit: In new code we are not putting a space between "template" and the following <>.

> Source/WebCore/bindings/js/JSDOMConstructor.h:62
> +    static JSDOMConstructor* create(JSC::VM& vm, JSC::Structure* structure, JSDOMGlobalObject& globalObject)

Can this take a reference to the structure instead of a pointer?

Can this return a reference to the new object instead of a pointer?

> Source/WebCore/bindings/js/JSDOMConstructor.h:69
> +    static JSC::Structure* createStructure(JSC::VM& vm, JSC::JSGlobalObject& globalObject, JSC::JSValue prototype)

Can this return a reference to the structure instead of a pointer?

> Source/WebCore/bindings/js/JSDOMConstructor.h:81
> +    void initializeProperties(JSC::VM&, JSDOMGlobalObject&) { }

I don’t understand the value of defining an inline private empty function and also writing code that calls it. We should omit both the function and calls to it unless there is some deeper strategy here.

> Source/WebCore/bindings/js/JSDOMConstructor.h:101
> +    static JSDOMNamedConstructor* create(JSC::VM& vm, JSC::Structure* structure, JSDOMGlobalObject& globalObject)

Can this take a reference to the structure instead of a pointer?

Can this return a reference to the new object instead of a pointer?

> Source/WebCore/bindings/js/JSDOMConstructor.h:108
> +    static JSC::Structure* createStructure(JSC::VM& vm, JSC::JSGlobalObject& globalObject, JSC::JSValue prototype)

Can this return a reference to the structure instead of a pointer?

> Source/WebCore/bindings/js/JSDOMConstructor.h:120
> +    void initializeProperties(JSC::VM&, JSDOMGlobalObject&) { }

I don’t understand the value of defining an inline private empty function and also writing code that calls it. We should omit both the function and calls to it.

> Source/WebCore/bindings/js/JSDOMConstructor.h:178
> -    JSObject* object = castedThis->createJSObject();
> +    JSC::JSObject* object = castedThis->createJSObject();

What are we fixing here by adding the explicit JSC prefix? Why did the code compile before?

On a separate point, I suggest using just auto or auto* instead of naming the type.

> Source/WebCore/bindings/js/JSImageConstructor.h:23
> +#include <runtime/JSCJSValue.h>

We don’t need to include this header. Even for a return value, it’s sufficient to forward-declare the JSC::JSValue class as we do with VM below. To actually call the function you need the JSValue class definition, but that’s true of VM too.

[youenn fablet]

Created attachment 263268 [details]
Patch for landing

[youenn fablet]

Thanks for the review!

> Generally I find these class templates easier to read if we put the function
> definitions all outside the class template definition. Even though the total
> file size is bigger. Inlining can be controlled with the inline keyword
> rather than by putting the functions to be inlined into the class definition.

Did the edits accordingly.

> I think I am having a little trouble seeing the big picture here and
> understanding the strategy. I don’t fully understand the patch. I do think
> it’s OK to use templates more so we can generate less code from the code
> generation script, as long as we don’t cause an explosion in compile time or
> binary size by doing that. But there is something peculiar about these new
> templates. It’s really unclear what part of the template goes into the
> header and what part is designed be specialized. Might even be worth
> commenting on that in the class template definition.

Added more documentation in the change log.

As of complexity/code size, basically, these templates only do what the code generator was doing.
I hope that the amount of code is the same, except that code generator is now generating less.

The idea is that we specialize some members:
- construct and initializeProperties functions.
- s_info data
construct has no default implementation.
initializeProperties has one as private constructors (one not generated by binding generator) may not care about adding specific properties.
If that is misleading, I can remove the default initializeProperties implementation, and add it where missing (streams reader and controller private constructors)

Currently, we almost need to tweak the binding generator, even for very specific cases that might be better handled by custom classes. But writing custom classes is not very attractive.
If we have more templates like that, it may be come a bit more attractive to directly author custom classes.

>> Source/WebCore/ChangeLog:18
>> +        A further patch should remove DOMConstructorWithDocument and DOMConstructorJSBuiltinObject. 
> 
> Typically it’s helpful to keep class template as small as possible. It can be useful to put code in non-template base classes, even if the only use of those classes is in a single class template. Thus I am not 100% sure it will be good to remove these.

DOMConstructorObject is the base class.
DOMConstructorWithDocument has only inline methods so it could be squashed.
DOMConstructorJSBuiltinObject has one method that is not inlined.
It may be worth keeping it or find a way to make the inlined method a function.

>> Source/WebCore/bindings/js/JSDOMConstructor.h:27
>> +template <typename JSClass> class JSDOMConstructorNotConstructable : public DOMConstructorObject {
> 
> I don’t understand why this is a template. Nothing in this class template seems to depend on the JSClass argument. Even if we do have some reason to need a class template, is everything in this template needed? It’s often valuable to put as much as possible into a base class. I suppose that’s the role of DOMConstructorObject here? Maybe the main point of using a template here is that we will explicitly specialize one of the function or data members? 
> 
> Tiny nit: In new code we are not putting a space between "template" and the following <>.

Sure, I should have added comments, like I did for JSBuiltinConstructor.

>> Source/WebCore/bindings/js/JSDOMConstructor.h:31
>> +    static JSDOMConstructorNotConstructable* create(JSC::VM& vm, JSC::Structure* structure, JSDOMGlobalObject& globalObject)
> 
> Can this take a reference to the structure instead of a pointer?
> 
> Can this return a reference to the new object instead of a pointer?

I tend to think so but I have not done the exercise.
Plan for a future patch.

>> Source/WebCore/bindings/js/JSDOMConstructor.h:48
>> +    void initializeProperties(JSC::VM&, JSDOMGlobalObject&) { }
> 
> I don’t understand the value of defining an inline private empty function and also writing code that calls it. We should omit both the function and calls to it.

This default behavior is useful for private constructors for which we do not really care about adding specific properties since they do not surface at JS level, like 
JSBuiltinReadableStreamControllerPrivateConstructor and JSBuiltinReadableStreamReaderPrivateConstructor.

Constructors generated by binding generator always redefine initializeProperties.

>> Source/WebCore/bindings/js/JSDOMConstructor.h:58
>> +template <typename JSClass> class JSDOMConstructor : public DOMConstructorObject {
> 
> Is there a way to save code here? This seems really similar to JSDOMConstructorNotConstructable but lots of code seems to be repeated.
> 
> Tiny nit: In new code we are not putting a space between "template" and the following <>.

Fixed the nit.
I will try to look at saving more code later on.
Note that we do not loose any ground with the current patch since the binding generator was doing the same thing.

>> Source/WebCore/bindings/js/JSDOMConstructor.h:178
>> +    JSC::JSObject* object = castedThis->createJSObject();
> 
> What are we fixing here by adding the explicit JSC prefix? Why did the code compile before?
> 
> On a separate point, I suggest using just auto or auto* instead of naming the type.

I do not recall whether it was a compilation error or a style edit.
auto is better anyway.

[WebKit Commit Bot]

Comment on attachment 263268 [details]
Patch for landing

Clearing flags on attachment: 263268

Committed r191176: <http://trac.webkit.org/changeset/191176>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #7)
> As of complexity/code size, basically, these templates only do what the code
> generator was doing.
> I hope that the amount of code is the same, except that code generator is
> now generating less.

We may be misunderstanding each other on this. I understood the point you are making. My point was that C++ compilers may take substantially longer to process class templates and instantiate the template classes from those templates than they would to process the individual classes that a code generator could generate.

[Darin Adler]

Comment on attachment 263268 [details]
Patch for landing

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

> Source/WebCore/bindings/js/JSDOMConstructor.h:32
> +    static JSC::Structure* createStructure(JSC::VM&, JSC::JSGlobalObject&, JSC::JSValue);

I noticed one tiny mistake that we should return and fix. I don’t think the meaning of JSC::JSValue is clear here without the argument name, prototype.

[Darin Adler]

Comment on attachment 263268 [details]
Patch for landing

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

> Source/WebCore/bindings/js/JSDOMConstructor.h:137
> +template<typename JSClass> void JSDOMConstructor<JSClass>::finishCreation(JSC::VM& vm, JSDOMGlobalObject& globalObject)

In the final patch you landed this was one of the only function templates without “inline”. Was that intentional? If not, I suggest adding the inline.

> Source/WebCore/bindings/js/JSDOMConstructor.h:144
> +template<typename JSClass> JSC::ConstructType JSDOMConstructor<JSClass>::getConstructData(JSC::JSCell*, JSC::ConstructData& constructData)

In the final patch you landed this was one of the only function templates without “inline”. Was that intentional? If not, I suggest adding the inline.

[youenn fablet]

Thanks for the additional points.
I filed bug 150238 to keep track of these.

[youenn fablet]

(In reply to comment #10)
> (In reply to comment #7)
> > As of complexity/code size, basically, these templates only do what the code
> > generator was doing.
> > I hope that the amount of code is the same, except that code generator is
> > now generating less.
> 
> We may be misunderstanding each other on this. I understood the point you
> are making. My point was that C++ compilers may take substantially longer to
> process class templates and instantiate the template classes from those
> templates than they would to process the individual classes that a code
> generator could generate.

I see.
Although I guess it could be monitored by bots statistics, one patch may not bring enough perf differences to be noticeable.

[Csaba Osztrogonác]

Comment on attachment 263268 [details]
Patch for landing

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4939
> +    if ($conditionalString) {
> +        push(@$outputArray, "template<> ConstructType ${constructorClassName}::getConstructData(JSCell*, ConstructData& constructData)\n");
> +        push(@$outputArray, "{\n");
> +        push(@$outputArray, "#if $conditionalString\n");
> +        push(@$outputArray, "    constructData.native.function = construct;\n");
> +        push(@$outputArray, "    return ConstructTypeHost;\n");
> +        push(@$outputArray, "#else\n");
> +        push(@$outputArray, "    return Base::getConstructData(cell, constructData);\n");
> +        push(@$outputArray, "#endif\n");
> +        push(@$outputArray, "}\n");
> +        push(@$outputArray, "\n");
> +    }

The #else case broke the build, I'm fixed it in bug150320.