Bug 149751 - Refactor WebCore JS builtins to prepare for automatic generation
Summary: Refactor WebCore JS builtins to prepare for automatic generation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on: 149877 149924 149955
Blocks: 149842
  Show dependency treegraph
 
Reported: 2015-10-02 09:25 PDT by youenn fablet
Modified: 2015-10-09 08:27 PDT (History)
5 users (show)

See Also:


Attachments
Patch (39.18 KB, patch)
2015-10-05 08:07 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (61.60 KB, patch)
2015-10-06 10:54 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch (47.97 KB, patch)
2015-10-07 04:14 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Patch for landing (47.96 KB, patch)
2015-10-07 05:00 PDT, Xabier Rodríguez Calvar
no flags Details | Formatted Diff | Diff
Updating DerivedSources.make rules (50.94 KB, patch)
2015-10-07 14:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Generating one file at a time in generate-jsbuiltins-allinone (51.80 KB, patch)
2015-10-07 23:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Removing automatic generation and adding previously generated files in trunk (35.37 KB, patch)
2015-10-09 05:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing mac build (43.75 KB, patch)
2015-10-09 06:36 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2015-10-02 09:25:08 PDT
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.
Comment 1 youenn fablet 2015-10-05 08:07:41 PDT
Created attachment 262436 [details]
Patch
Comment 2 WebKit Commit Bot 2015-10-05 08:10:00 PDT
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.
Comment 3 Darin Adler 2015-10-06 08:35:33 PDT
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.
Comment 4 Xabier Rodríguez Calvar 2015-10-06 10:54:11 PDT
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.
Comment 5 Xabier Rodríguez Calvar 2015-10-06 11:01:24 PDT
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 :)
Comment 6 youenn fablet 2015-10-06 11:04:01 PDT
(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.
Comment 7 Xabier Rodríguez Calvar 2015-10-07 04:14:18 PDT
Created attachment 262593 [details]
Patch

Changed the class names, according to the conversation with Youenn.
Comment 8 WebKit Commit Bot 2015-10-07 04:30:29 PDT
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
Comment 9 Xabier Rodríguez Calvar 2015-10-07 05:00:30 PDT
Created attachment 262594 [details]
Patch for landing
Comment 10 youenn fablet 2015-10-07 05:09:15 PDT
(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.
Comment 11 WebKit Commit Bot 2015-10-07 05:53:05 PDT
Comment on attachment 262594 [details]
Patch for landing

Clearing flags on attachment: 262594

Committed r190664: <http://trac.webkit.org/changeset/190664>
Comment 12 WebKit Commit Bot 2015-10-07 05:53:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Simon Fraser (smfr) 2015-10-07 08:43:50 PDT
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
Comment 14 youenn fablet 2015-10-07 08:56:27 PDT
(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?
Comment 15 WebKit Commit Bot 2015-10-07 09:08:02 PDT
Re-opened since this is blocked by bug 149877
Comment 16 Darin Adler 2015-10-07 09:09:14 PDT
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?
Comment 17 Xabier Rodríguez Calvar 2015-10-07 09:13:35 PDT
(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
Comment 18 youenn fablet 2015-10-07 14:01:03 PDT
Created attachment 262639 [details]
Updating DerivedSources.make rules
Comment 19 youenn fablet 2015-10-07 23:33:31 PDT
Created attachment 262677 [details]
Generating one file at a time in generate-jsbuiltins-allinone
Comment 20 youenn fablet 2015-10-07 23:42:01 PDT
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.
Comment 21 Xabier Rodríguez Calvar 2015-10-08 02:00:41 PDT
Comment on attachment 262677 [details]
Generating one file at a time in generate-jsbuiltins-allinone

Clearing flags
Comment 22 youenn fablet 2015-10-08 02:12:02 PDT
Not sure why it was set as RESOLVED. Reopening it manually.
Comment 23 youenn fablet 2015-10-08 02:36:27 PDT
(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
Comment 24 WebKit Commit Bot 2015-10-08 02:56:02 PDT
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>
Comment 25 WebKit Commit Bot 2015-10-08 02:56:07 PDT
All reviewed patches have been landed.  Closing bug.
Comment 26 WebKit Commit Bot 2015-10-08 07:51:18 PDT
Re-opened since this is blocked by bug 149924
Comment 27 youenn fablet 2015-10-08 08:02:00 PDT
(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.
Comment 28 youenn fablet 2015-10-08 08:03:42 PDT
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'
Comment 30 Darin Adler 2015-10-08 08:36:16 PDT
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?
Comment 31 youenn fablet 2015-10-08 09:00:21 PDT
(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.
Comment 32 Brian Burg 2015-10-08 09:11:14 PDT
(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.
Comment 33 youenn fablet 2015-10-08 10:26:45 PDT
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.
Comment 34 youenn fablet 2015-10-08 10:28:40 PDT
(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.
Comment 35 Brian Burg 2015-10-08 10:35:36 PDT
(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.
Comment 36 youenn fablet 2015-10-09 01:10:51 PDT
Reopening it manually...
Comment 37 youenn fablet 2015-10-09 05:09:06 PDT
Created attachment 262762 [details]
Removing automatic generation and adding previously generated files in trunk
Comment 38 WebKit Commit Bot 2015-10-09 05:12:07 PDT
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.
Comment 39 WebKit Commit Bot 2015-10-09 05:57:44 PDT
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
Comment 40 youenn fablet 2015-10-09 06:36:49 PDT
Created attachment 262766 [details]
Fixing mac build
Comment 41 WebKit Commit Bot 2015-10-09 06:39:42 PDT
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.
Comment 42 WebKit Commit Bot 2015-10-09 07:22:30 PDT
Comment on attachment 262766 [details]
Fixing mac build

Clearing flags on attachment: 262766

Committed r190794: <http://trac.webkit.org/changeset/190794>
Comment 43 WebKit Commit Bot 2015-10-09 07:22:36 PDT
All reviewed patches have been landed.  Closing bug.