Bug 159674 – WebCore builtins should be created lazily

• Component: WebCore JavaScript
• Status: NEW

[youenn fablet]

DOM constructors are created lazily, on first access by user script.
It might be good to allow the same thing for JS builtins.
One possibility is to link the JS builtins not to the global object but to the related constructor object.

[youenn fablet]

Created attachment 283410 [details]
Patch

[youenn fablet]

Created attachment 283417 [details]
Fixing isDisturbed

[Blaze Burg]

Comment on attachment 283417 [details]
Fixing isDisturbed

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

I have reviewed the python bindings generator changes. Please have other reviewers look at the Perl code generator changes, as well as the actual logic / builtins changes.

> Source/JavaScriptCore/ChangeLog:8
> +        Adding support for @constructorPrivate, the idea being that all functions annotated with this annotation being

This sentence is hard to read. Maybe "Add support for @constructorPrivate. Functions with this annotation are added to the related constructor as private properties." Also, you should paste the rationale for the change (already on the bugzilla bug) at the top of the changelog before explaining the patch contents.

> Source/JavaScriptCore/ChangeLog:22
> +        (BuiltinsWrapperHeaderGenerator.generate_constructor): Always calling exportNames now that any built-in may

For function-level commentary longer than a few words, it's usually best to just start on a newline to make it readable. That also makes it flow better with a blank line between commentary and the next function line.

> Source/WebCore/ChangeLog:8
> +        Covered by existing ReadableStream tests.

Please describe the changes to WebCore at a high level.

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:173
> +        constructor_code_lines.append("#define %(macroPrefix)s_FOREACH_%(objectMacro)s_BUILTIN_CONSTRUCTOR_PRIVATE_CODE(macro) \\" % args)

I think it would be better to extract the loop body into a separate function, filter the functions to be generated per group (all, constructor, globals) rather than having conditionals inside the loop body, then generate each sequentially in the order they are listed in the code. So it would be something like:

def _generate_macros_for_functions(self, functions, macro_name_suffix):
...

constructor_functions = filter(self.object.functions, lambda f: f.is_global_private)
...

lines.extend(self._generate_macros_for_functions(constructor_functions, "BUILTIN_CONSTRUCTOR_PRIVATE_CODE"))



Throughout the new python generators I tried really hard to make the generator's control flow match the generated text, even at the expense of mind-numbingly simple and sometimes redundant control flow. This change violates that approach to code structure. I think we should try to preserve that style as it is key to keeping the generators simple to follow.

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:202
> +

There is no fallback case here. Should it return False?

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:221
> +        unique_names = list(set([function.function_name for function in self.object.functions if self._is_private_function(function)]))

Nit: 'unique_function_names'

[youenn fablet]

Created attachment 283523 [details]
Improving readability

[youenn fablet]

Thanks for the comments, I updated the patch accordingly.
The built-in generator updated code looks better and I'll try to stick with the code style there.

(In reply to comment #3)
> Comment on attachment 283417 [details]
> Fixing isDisturbed
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283417&action=review
> 
> I have reviewed the python bindings generator changes. Please have other
> reviewers look at the Perl code generator changes, as well as the actual
> logic / builtins changes.
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Adding support for @constructorPrivate, the idea being that all functions annotated with this annotation being
> 
> This sentence is hard to read. Maybe "Add support for @constructorPrivate.
> Functions with this annotation are added to the related constructor as
> private properties." Also, you should paste the rationale for the change
> (already on the bugzilla bug) at the top of the changelog before explaining
> the patch contents.
> 
> > Source/JavaScriptCore/ChangeLog:22
> > +        (BuiltinsWrapperHeaderGenerator.generate_constructor): Always calling exportNames now that any built-in may
> 
> For function-level commentary longer than a few words, it's usually best to
> just start on a newline to make it readable. That also makes it flow better
> with a blank line between commentary and the next function line.
> 
> > Source/WebCore/ChangeLog:8
> > +        Covered by existing ReadableStream tests.
> 
> Please describe the changes to WebCore at a high level.
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:173
> > +        constructor_code_lines.append("#define %(macroPrefix)s_FOREACH_%(objectMacro)s_BUILTIN_CONSTRUCTOR_PRIVATE_CODE(macro) \\" % args)
> 
> I think it would be better to extract the loop body into a separate
> function, filter the functions to be generated per group (all, constructor,
> globals) rather than having conditionals inside the loop body, then generate
> each sequentially in the order they are listed in the code. So it would be
> something like:
> 
> def _generate_macros_for_functions(self, functions, macro_name_suffix):
> ...
> 
> constructor_functions = filter(self.object.functions, lambda f:
> f.is_global_private)
> ...
> 
> lines.extend(self._generate_macros_for_functions(constructor_functions,
> "BUILTIN_CONSTRUCTOR_PRIVATE_CODE"))
> 
> 
> 
> Throughout the new python generators I tried really hard to make the
> generator's control flow match the generated text, even at the expense of
> mind-numbingly simple and sometimes redundant control flow. This change
> violates that approach to code structure. I think we should try to preserve
> that style as it is key to keeping the generators simple to follow.
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:202
> > +
> 
> There is no fallback case here. Should it return False?
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_generate_separate_header.py:221
> > +        unique_names = list(set([function.function_name for function in self.object.functions if self._is_private_function(function)]))
> 
> Nit: 'unique_function_names'

[Geoffrey Garen]

I'd love to see builtins-related code move away from preprocessor macros and nested preprocessor macros. I have a hard time reading code like that, and of course it's impossible to interact with code like that in a debugger. This is doubly true in a context where we have access to a programmatic code generator.

Maybe this comment is out of scope for this patch, but I'd like us to keep it in mind in the future.

[youenn fablet]

Created attachment 284203 [details]
Removing macro use

[youenn fablet]

Created attachment 284207 [details]
Integrated missing change to ReadableStream

[youenn fablet]

(In reply to comment #6)
> I'd love to see builtins-related code move away from preprocessor macros and
> nested preprocessor macros. I have a hard time reading code like that, and
> of course it's impossible to interact with code like that in a debugger.
> This is doubly true in a context where we have access to a programmatic code
> generator.
> 
> Maybe this comment is out of scope for this patch, but I'd like us to keep
> it in mind in the future.

Thanks for the comment.
I updated the code related to @constructorPrivate accordingly.

For the changes related to exporting names, I kept the macro way.
A future patch should remove macro uses from the wrapper classes, including exporting names. This would not improve JSC generated code but at some point we should unify the WebCore and JSC approaches.

[Brady Eidson]

Comment on attachment 284207 [details]
Integrated missing change to ReadableStream

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.