Bug 149751 – Refactor WebCore JS builtins to prepare for automatic generation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

For each new JS builtin file, there are a few edits that are needed in the build system, WebCoreJSClientData.h and WebCoreJSBuiltins.cpp.
It might be good if only editing the build system would be needed.
That would mean auto generating WebCoreJSBuiltins.cpp and the part of WebCoreJSClientData.h related to JS builtins.

[youenn fablet]

Created attachment 262436 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/bindings/js/JSDOMWindowBase.cpp:89:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 15 files


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

[Darin Adler]

Comment on attachment 262436 [details]
Patch

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

> Source/WebCore/bindings/js/WebCoreJSClientData.h:33
>  class WebCoreJSClientData : public JSC::VM::ClientData {

Since this is in the WebCore namespace, it seems a little excessive to include the words “WebCore” in the name of the class.

> Source/WebCore/generate-js-builtins-allinone:134
> +class WebCoreJSBuiltinFunctions {

Since this is in the WebCore namespace, it seems a little excessive to include the words “WebCore” in the name of the class.

[Xabier Rodríguez Calvar]

Created attachment 262526 [details]
Patch

After talking to Youenn we thought it was better to deambiguate the class names from the JSC ones and have the WebCore prefix in them. Anyway, we renamed them from WebCoreJS to WebCore.

[Xabier Rodríguez Calvar]

It seems I misunderstood Youenn, maybe he can clarify how he wants the names and Darin can bless it so that we can land it :)

[youenn fablet]

(In reply to comment #5)
> It seems I misunderstood Youenn, maybe he can clarify how he wants the names
> and Darin can bless it so that we can land it :)

Meaning the reverse, disambiguate the filenames, not the class names.
I will update the patch accordingly today or tomorrow if there is no objection.

[Xabier Rodríguez Calvar]

Created attachment 262593 [details]
Patch

Changed the class names, according to the conversation with Youenn.

[WebKit Commit Bot]

Comment on attachment 262593 [details]
Patch

Rejecting attachment 262593 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 262593, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/253723

[Xabier Rodríguez Calvar]

Created attachment 262594 [details]
Patch for landing

[youenn fablet]

(In reply to comment #9)
> Created attachment 262594 [details]
> Patch for landing

Name changes
- PrivateWebCoreJSBuiltinFunctions -> JSBuiltinInternalFunctions
- WebCoreJSBuiltinFunctions -> JSBuiltinFunctions
- WebCoreJSClientData -> JSClientData

For the latter, JSVMClientData might have been somehow better.

[WebKit Commit Bot]

Comment on attachment 262594 [details]
Patch for landing

Clearing flags on attachment: 262594

Committed r190664: <http://trac.webkit.org/changeset/190664>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

My build is broken after this:
CpHeader PrivateWebCoreJSBuiltins
error: /Volumes/DataSSD/Development/apple/webkit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h: No such file or directory

[youenn fablet]

(In reply to comment #13)
> My build is broken after this:
> CpHeader PrivateWebCoreJSBuiltins
> error:
> /Volumes/DataSSD/Development/apple/webkit/OpenSource/WebKitBuild/Debug/
> DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h: No such file or directory

Probably related to DerivedSources.make.
Can you try a clean build?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 149877

[Darin Adler]

I don’t think it’s reasonable to ask everyone to do a clean build. Perhaps this would be fixed by touching the generate-js-builtins-allinone source file one more time? We should look at his build log to see if generate-js-builtins-allinone failed?

[Xabier Rodríguez Calvar]

(In reply to comment #16)
> I don’t think it’s reasonable to ask everyone to do a clean build. Perhaps
> this would be fixed by touching the generate-js-builtins-allinone source
> file one more time? We should look at his build log to see if
> generate-js-builtins-allinone failed?

Actually it is not reasonable at all as you'd need a half built WebKit to build from scratch.

The problem happens at:

CpHeader /Users/calvaris/WebKit/WebKitBuild/Release/DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h /Users/calvaris/WebKit/WebKitBuild/Release/WebCore.framework/Versions/A/PrivateHeaders/PrivateWebCoreJSBuilt\
ins.h
    cd /Users/calvaris/WebKit/Source/WebCore
    builtin-copy -exclude .DS_Store -exclude CVS -exclude .svn -exclude .git -exclude .hg -strip-debug-symbols -strip-tool /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/\
strip -resolve-src-symlinks /Users/calvaris/WebKit/WebKitBuild/Release/DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h /Users/calvaris/WebKit/WebKitBuild/Release/WebCore.framework/Versions/A/PrivateHeaders
error: /Users/calvaris/WebKit/WebKitBuild/Release/DerivedSources/WebCore/PrivateWebCoreJSBuiltins.h: No such file or directory

I tried to add:

diff --git a/Source/WebCore/DerivedSources.make b/Source/WebCore/DerivedSources.make
index a5967e7..8d12cc6 100644
--- a/Source/WebCore/DerivedSources.make
+++ b/Source/WebCore/DerivedSources.make
@@ -1260,7 +1260,9 @@ WEBCORE_JS_BUILTINS = \
     $(WebCore)/Modules/streams/ReadableStreamReader.js \
 #
 
-all : WebCoreJSBuiltins.cpp $(WEBCORE_JS_BUILTINS:%.js=%Builtins.cpp)
+all : PrivateWebCoreJSBuiltins.h WebCoreJSBuiltins.cpp $(WEBCORE_JS_BUILTINS:%.js=%Builtins.cpp)
+
+PrivateWebCoreJSBuiltins.h: WebCoreJSBuiltins.cpp
 
 WebCoreJSBuiltins.cpp: $(WEBCORE_JS_BUILTINS) $(WebCore)/generate-js-builtins-allinone
        $(PYTHON) $(WebCore)/generate-js-builtins-allinone $(WEBCORE_JS_BUILTINS)  --output_dir .

but it didn't work either

[youenn fablet]

Created attachment 262639 [details]
Updating DerivedSources.make rules

[youenn fablet]

Created attachment 262677 [details]
Generating one file at a time in generate-jsbuiltins-allinone

[youenn fablet]

The updated DerivedSources.make rules should hopefully cover the issue encountered by smfr. I played with the generated Builtins.h/.cpp files and they seem to be correctly regenerated when needed.

That said, last patch failed on mac due to a different issue from the one reported by smfr:
---------------------
Traceback (most recent call last):
  File "WebCore/generate-js-builtins-allinone", line 232, in <module>
    copytempfile(output_base + ".h")
  File "WebCore/generate-js-builtins-allinone", line 225, in copytempfile
    if (not os.path.exists(output)) or (not filecmp.cmp(output + ".tmp", output, shallow=False)):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/filecmp.py", line 42, in cmp
rm -f CommandLineAPIModuleSource.min.js
    s1 = _sig(os.stat(f1))
OSError: [Errno 2] No such file or directory: './WebCoreJSBuiltins.h.tmp'
---------------------

This is somehow strange since the script should have generated that .h.tmp file a few lines before.
I added file flushing in the script just in case.

[Xabier Rodríguez Calvar]

Comment on attachment 262677 [details]
Generating one file at a time in generate-jsbuiltins-allinone

Clearing flags

[youenn fablet]

Not sure why it was set as RESOLVED. Reopening it manually.

[youenn fablet]

(In reply to comment #16)
> I don’t think it’s reasonable to ask everyone to do a clean build. Perhaps
> this would be fixed by touching the generate-js-builtins-allinone source
> file one more time? We should look at his build log to see if
> generate-js-builtins-allinone failed?

From IRC discussions, a header file was not generated, meaning that generate-js-builtins-allinone was probably not kicking in.
I am not exactly sure why since mac bots were fine.

I am now relanding an updated patch:
- Some changes in DerivedSources.make rules that hopefully will fix smfr issue.
- Renaming WebCoreJSClientData as JSVMClientData.
- Generating one file at a time in generate-js-builtins-allinone

[WebKit Commit Bot]

Comment on attachment 262677 [details]
Generating one file at a time in generate-jsbuiltins-allinone

Clearing flags on attachment: 262677

Committed r190716: <http://trac.webkit.org/changeset/190716>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 149924

[youenn fablet]

(In reply to comment #26)
> Re-opened since this is blocked by bug 149924

The following issue is now happening from time to time on Mac bots.

---------------------
Generating WebCore JS builtins wrapper files.
Traceback (most recent call last):
  File "WebCore/generate-js-builtins-allinone", line 127, in <module>
    copytempfile(output_base + ".cpp")
  File "WebCore/generate-js-builtins-allinone", line 74, in copytempfile
    os.remove(output + ".tmp")
OSError: [Errno 2] No such file or directory: './WebCoreJSBuiltins.cpp.tmp'
make: *** [WebCoreJSBuiltins.h] Error 1
---------------------

generate-js-builtins-allinone should be generating WebCoreJSBuiltins.cpp.tmp.
Then it should try to compare it with WebCoreJSBuiltins.cpp
If they are the same, then WebCoreJSBuiltins.cpp.tmp is deleted.
This step seemed to fail.

[youenn fablet]

Another trace found in an EWS mac bot:

Traceback (most recent call last):
  File "WebCore/generate-js-builtins-allinone", line 127, in <module>
    copytempfile(output_base + ".cpp")
  File "WebCore/generate-js-builtins-allinone", line 69, in copytempfile
    if (not os.path.exists(output)) or (not filecmp.cmp(output + ".tmp", output, shallow=False)):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/filecmp.py", line 42, in cmp
    s1 = _sig(os.stat(f1))
OSError: [Errno 2] No such file or directory: './WebCoreJSBuiltins.cpp.tmp'

[youenn fablet]

Some build log links:
https://webkit-queues.webkit.org/results/257622
https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Build%29/builds/19177/steps/compile-webkit/logs/stdio

[Darin Adler]

A comment about the WebCoreJSBuiltins.cpp.tmp strategy (not an explanation of the build failures, but a separate issue):

This technique of only touching a file if there is a change doesn’t work well with makefiles. If a build rule doesn’t regenerate the file then it will keep getting triggered over and over again each time you make, so it’s typically not practical to optimize the case where there is no change to have it not touch the output file.

The desire to optimize in this way comes up over and over again, so we’ve run into that problem over and over again. Maybe there’s some trick here used to avoid that problem?

[youenn fablet]

(In reply to comment #29)
> Some build log links:
> https://webkit-queues.webkit.org/results/257622
> https://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Build%29/
> builds/19177/steps/compile-webkit/logs/stdio

Problem is probably again in DerivedSources.make
It seems generate-js-builtins-allinone is called twice, explaining the issue.

[Blaze Burg]

(In reply to comment #30)
> A comment about the WebCoreJSBuiltins.cpp.tmp strategy (not an explanation
> of the build failures, but a separate issue):
> 
> This technique of only touching a file if there is a change doesn’t work
> well with makefiles. If a build rule doesn’t regenerate the file then it
> will keep getting triggered over and over again each time you make, so it’s
> typically not practical to optimize the case where there is no change to
> have it not touch the output file.
> 
> The desire to optimize in this way comes up over and over again, so we’ve
> run into that problem over and over again. Maybe there’s some trick here
> used to avoid that problem?

We do something similar in the Inspector protocol bindings generator, and it has worked pretty well once the bugs were flushed out. I'll comment specifically on the patch in a minute.

[youenn fablet]

I may retry landing it tomorrow, but only the part that does not changing DerivedSources.make.
Basically that means upstreaming the refactoring, the files generated by generate-js-builtins-allinone but not the generate-js-builtins-allinone script.

This will allow starting working on WritableStream as well as reducing the patch that would upstream generate-js-builtins-allinone.

Please let me know whether the reduced patch would need a new review cycle.
Otherwise, I will land it directly tomorrow.

[youenn fablet]

(In reply to comment #32)
> (In reply to comment #30)
> > A comment about the WebCoreJSBuiltins.cpp.tmp strategy (not an explanation
> > of the build failures, but a separate issue):
> > 
> > This technique of only touching a file if there is a change doesn’t work
> > well with makefiles. If a build rule doesn’t regenerate the file then it
> > will keep getting triggered over and over again each time you make, so it’s
> > typically not practical to optimize the case where there is no change to
> > have it not touch the output file.
> > 
> > The desire to optimize in this way comes up over and over again, so we’ve
> > run into that problem over and over again. Maybe there’s some trick here
> > used to avoid that problem?
> 
> We do something similar in the Inspector protocol bindings generator, and it
> has worked pretty well once the bugs were flushed out. I'll comment
> specifically on the patch in a minute.

That sounds interesting.

Using CMake (with ccache bonus) would also help reducing this issue and maybe remove the need for this particular optimization.

[Blaze Burg]

(In reply to comment #33)
> I may retry landing it tomorrow, but only the part that does not changing
> DerivedSources.make.
> Basically that means upstreaming the refactoring, the files generated by
> generate-js-builtins-allinone but not the generate-js-builtins-allinone
> script.
> 
> This will allow starting working on WritableStream as well as reducing the
> patch that would upstream generate-js-builtins-allinone.
> 
> Please let me know whether the reduced patch would need a new review cycle.
> Otherwise, I will land it directly tomorrow.

I would recommend proceeding with the "manual" approach for the time being. I'm looking into making generate-js-builtins look more like the inspector generator (i.e., has regression tests, doesn't use .tmp pattern), but this will be a bit of work.

[youenn fablet]

Reopening it manually...

[youenn fablet]

Created attachment 262762 [details]
Removing automatic generation and adding previously generated files in trunk

[WebKit Commit Bot]

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


ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:30:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:39:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:44:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 21 files


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

[WebKit Commit Bot]

Comment on attachment 262762 [details]
Removing automatic generation and adding previously generated files in trunk

Rejecting attachment 262762 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
/usr/bin/strip -resolve-src-symlinks /Volumes/Data/EWS/WebKit/Source/WebCore/Modules/indexeddb/IDBDatabaseMetadata.h /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/Versions/A/PrivateHeaders

** BUILD FAILED **


The following build commands failed:
	CpHeader /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/WebCoreJSBuiltinInternals.h /Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/Versions/A/PrivateHeaders/WebCoreJSBuiltinInternals.h
(1 failure)

Full output: http://webkit-queues.webkit.org/results/260970

[youenn fablet]

Created attachment 262766 [details]
Fixing mac build

[WebKit Commit Bot]

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


ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:30:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:39:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:44:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/WebCoreJSBuiltinInternals.h:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 21 files


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

[WebKit Commit Bot]

Comment on attachment 262766 [details]
Fixing mac build

Clearing flags on attachment: 262766

Committed r190794: <http://trac.webkit.org/changeset/190794>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.