Bug 155871 - [Win] Improve CMake build performance
Summary: [Win] Improve CMake build performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC All
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on: 157798
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-24 22:18 PDT by Brent Fulgham
Modified: 2016-05-17 09:51 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.46 KB, patch)
2016-03-24 22:30 PDT, Brent Fulgham
achristensen: review+
Details | Formatted Diff | Diff
Patch v2 (16.41 KB, patch)
2016-03-24 22:59 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2016-03-24 22:18:11 PDT
The time required to build our Windows WebKit builds has increased significantly since we switched to CMake.

While investigating this I noticed that we no longer use the DerivedSources.cpp "all in one" file to drive the build of our Derived Sources. We started using this years ago to address long link times and other build problems, so I don't think we should have stopped using it.

This patch corrects the build as follows:
1. It uses "DerivedSources.cpp"
2. To avoid breaking CMake dependency analysis, we revise the VS projects to show the header for each auto generated file, which forces CMake to make sure they are kept in sync, without causing VS to actually build the files.
3. Updates "DerivedSources.cpp" with some new files that were added since the last time it was updated.

Although its annoying to keep DerivedSources.cpp in sync with the other pieces of the build system, it's no worse than the dozen or so other "all in one" files we already use (on multiple ports). Plus, it's unlikely we will break this in the future because a missing file just means that VS builds it separately, slightly slowing the build. Of course, removing an IDL file without updating the DerivedSources file could break the build, but EWS should show that quite clearly.
Comment 1 Brent Fulgham 2016-03-24 22:30:00 PDT
Created attachment 274884 [details]
Patch
Comment 2 WebKit Commit Bot 2016-03-24 22:32:41 PDT
Attachment 274884 [details] did not pass style-queue:


ERROR: Source/WebCore/DerivedSources.cpp:349:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/DerivedSources.cpp:378:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/DerivedSources.cpp:412:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/DerivedSources.cpp:439:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/DerivedSources.cpp:590:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alex Christensen 2016-03-24 22:49:30 PDT
Comment on attachment 274884 [details]
Patch

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

r=me

> Source/WebCore/CMakeLists.txt:3805
> +    set(WebCore_SOURCES ${newSources})
> +    list(APPEND WebCore_SOURCES DerivedSources.cpp)

This can be done in one command, like is done with the other call to PROCESS_ALLINONE_FILE

> Source/cmake/WebKitMacros.cmake:374
> +            string(REGEX REPLACE "(.*)\\.cpp" "\\1" _allin_no_ext ${_allin})
> +            string(REGEX REPLACE ";([^;]*/)${_allin_no_ext}\\.cpp;" ";\\1${_allin_no_ext}.h;" _new_result "${${_result_file_list}};")

A comment here would be nice.  This is hard to read and tell what is going on.
# For DerivedSources.cpp, we still need the derived sources to be generated, but we do not want them to be compiled separately, 
# so we add the header to the result file list.
Comment 4 Brent Fulgham 2016-03-24 22:59:25 PDT
Created attachment 274887 [details]
Patch v2
Comment 5 Brent Fulgham 2016-03-25 08:38:41 PDT
(In reply to comment #3)
> Comment on attachment 274884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274884&action=review
> 
> r=me
> 
> > Source/WebCore/CMakeLists.txt:3805
> > +    set(WebCore_SOURCES ${newSources})
> > +    list(APPEND WebCore_SOURCES DerivedSources.cpp)
> 
> This can be done in one command, like is done with the other call to
> PROCESS_ALLINONE_FILE

OK!

> > Source/cmake/WebKitMacros.cmake:374
> > +            string(REGEX REPLACE "(.*)\\.cpp" "\\1" _allin_no_ext ${_allin})
> 
> A comment here would be nice.  This is hard to read and tell what is going
> on.

Will do!
Comment 6 Brent Fulgham 2016-03-25 09:16:06 PDT
Committed in r198669. <https://trac.webkit.org/r198669>