Bug 160115

Summary: Adding a new WebCore JavaScript built-in source file does not trigger rebuild of WebCoreJSBuiltins*
Product: WebKit Reporter: Darin Adler <darin>
Component: BindingsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, keith_miller, mark.lam, mitz, msaboff, saam, sam, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Darin Adler
Reported 2016-07-22 23:46:28 PDT
Addng a new WebCore JavaScript built-in source file does not trigger rebuild of WebCoreJSBuiltins*
Attachments
Patch (4.21 KB, patch)
2016-07-22 23:49 PDT, Darin Adler
no flags
Patch for landing (4.19 KB, patch)
2016-07-24 03:24 PDT, youenn fablet
no flags
Darin Adler
Comment 1 2016-07-22 23:49:49 PDT
Darin Adler
Comment 2 2016-07-22 23:51:15 PDT
I ran into this when I checked out the latest sources but they failed to link because XMLHttpRequest was not included in WebCoreJSBuiltins.cpp.
youenn fablet
Comment 3 2016-07-23 00:17:03 PDT
Comment on attachment 284405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284405&action=review Thanks! > Source/WebCore/DerivedSources.make:1334 > + $(PYTHON) $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py '$(WebCore_BUILTINS_SOURCES)' $@ Why is this needed? > Source/WebCore/DerivedSources.make:1339 > +$(firstword $(WebCore_BUILTINS_WRAPPERS)): WebCore_BUILTINS_SOURCES_LIST $(BUILTINS_GENERATOR_SCRIPTS) WebCore_BUILTINS_DEPENDENCIES_LIST Cannot we just use $(WebCore_BUILTINS_SOURCES) here?
youenn fablet
Comment 4 2016-07-24 03:24:02 PDT
Created attachment 284444 [details] Patch for landing
youenn fablet
Comment 5 2016-07-24 03:24:29 PDT
(In reply to comment #4) > Created attachment 284444 [details] > Patch for landing Updated bug title in log.
WebKit Commit Bot
Comment 6 2016-07-24 04:11:05 PDT
Comment on attachment 284444 [details] Patch for landing Clearing flags on attachment: 284444 Committed r203661: <http://trac.webkit.org/changeset/203661>
WebKit Commit Bot
Comment 7 2016-07-24 04:11:10 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 8 2016-07-24 10:27:22 PDT
Comment on attachment 284405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284405&action=review >> Source/WebCore/DerivedSources.make:1334 >> + $(PYTHON) $(JavaScriptCore_SCRIPTS_DIR)/UpdateContents.py '$(WebCore_BUILTINS_SOURCES)' $@ > > Why is this needed? Because generating the wrappers depends on the *list* of JS files, not on the contents of those files, and further it needs to depend on removals as well as additions to that list. This technique gives us a file that changes when the filenames in the list change. >> Source/WebCore/DerivedSources.make:1339 >> +$(firstword $(WebCore_BUILTINS_WRAPPERS)): WebCore_BUILTINS_SOURCES_LIST $(BUILTINS_GENERATOR_SCRIPTS) WebCore_BUILTINS_DEPENDENCIES_LIST > > Cannot we just use $(WebCore_BUILTINS_SOURCES) here? No. Doing that would rebuild: 1) Too often. Any time we changed the contents of one of the sources. 2) Not often enough. Would not trigger a rebuild if we removed a file from the list.
youenn fablet
Comment 9 2016-07-25 13:27:15 PDT
(In reply to comment #8) > Comment on attachment 284405 [details] > Patch > > >> Source/WebCore/DerivedSources.make:1339 > >> +$(firstword $(WebCore_BUILTINS_WRAPPERS)): WebCore_BUILTINS_SOURCES_LIST $(BUILTINS_GENERATOR_SCRIPTS) WebCore_BUILTINS_DEPENDENCIES_LIST > > > > Cannot we just use $(WebCore_BUILTINS_SOURCES) here? > > No. Doing that would rebuild: > > 1) Too often. Any time we changed the contents of one of the sources. I see the point. That said, even if sources are changed, like adding a function, the header/source files will remain totally unchanged (lazy writer). So the perf loss is only the time spent to run the python generator. It does not trigger any compilation. In the future, if we remove the use of macros, we might want to update wrapper cpp files according source file content. We can revisit this if we ever go down that path Also, it might be good to think of the best way to limit the recompilation of cpp files in case of adding/removing a function. > 2) Not often enough. Would not trigger a rebuild if we removed a file from > the list. OK
Note You need to log in before you can comment on or make changes to this bug.