Summary: | [CMake] Enable pre-compiled headers (PCH) | ||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Alberto Lopez Perez <clopez> | ||||||||||||||||||||||||||||||||||||||||||||||||
Component: | Tools / Tests | Assignee: | Adrian Perez <aperez> | ||||||||||||||||||||||||||||||||||||||||||||||||
Status: | ASSIGNED --- | ||||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | aboya, annulen, aperez, buildbot, calvaris, don.olmstead, ews-watchlist, gyuyoung.kim, Hironori.Fujii, ivlev.igor, keith_miller, mark.lam, mcatanzaro, mrobinson, msaboff, ossy, pnormand, ryuan.choi, saam, sergio, tzagallo, yoshiaki.jitsukawa, zan | ||||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=176076 https://bugs.webkit.org/show_bug.cgi?id=248202 |
||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 231905 | ||||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Carlos Alberto Lopez Perez
2014-12-09 02:43:45 PST
Created attachment 242904 [details]
WIP patch. Enables pre-compiled headers on WTF and JSC. Fails on WebCore (commented out)
WIP: It enables pre-compiled headers on WTF and JSC. Fails on WebCore (its commented out). I didn't get performance measures yet, so I'm not sure if this is worth. Test it by building with --cmakeargs="-DENABLE_PRECOMPILED_HEADERS=true". Check the main ChangeLog in the patch for more details.
Created attachment 319615 [details]
Patch
Here goes another attempt at this, with a new patch which works well for both
JavaScriptCore and WebCore. The approach followed is to reuse the existing
prefix headers, adding system includes to them for GTK/WPE, and using them as
input for creation of the precompiled header. This way:
- Only headers which are known to not change often get precompiled (that is:
system headers, and those from the internal JHBuild for development builds).
- We avoid breaking the build (super important!)
With the patch applied, with Clang, without CCache/IceCC, and using
“webkit-build --release --gtk --makeargs=WebCore”, build times are:
- ~39 minutes WITH precompiled headers.
- ~44 minutes WITHOUT precompiled headers.
This is certainly promising, and I already have some changes locally to also
enable precompiled headers for other targets (mainly WebKit, also WebProcess,
NetworkProcess, etc).
My main (and only) doubt is: Is it okay to reuse the existing prefix headers
for this?
(BTW, the builds I made here work fine, MiniBrowser works, and I also tried
running some layout tests (which worked), but I didn't wait for a full run to
finish.)
One comment: to use precompiled headers *and* CCache at the same time, one needs to: export CCACHE_SLOPPINESS=pch_defines,time_macros Cotire will emit a warning when CMake runs if it doesn't find the environment variable set. I think we should use https://github.com/larsch/cmake-precompiled-header for PCH and static *AllInOne.cpp files for unity builds. Comment on attachment 319615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319615&action=review Awesome! I did not realize precompiled headers existed on Linux. The necessity for the CCACHE_SLOPPINESS environment variable is the main blocker here. It has to work without needing any environment variables. ccache gets pulled in with the @developer-tools group on Fedora so it's expected to be present, but developers should not be expected to know what it is. > Source/JavaScriptCore/CMakeLists.txt:1607 > +if (COMMAND cotire) How does this conditional work? It's testing for what exactly? > Source/JavaScriptCore/CMakeLists.txt:1611 > + COTIRE_PREFIX_HEADER_INCLUDE_PATH "${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesGTK/Root;${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesWPE/Root" r- also for the hardcoded paths. This is at least easy to fix; you can define some CMake variable differently in OptionsGTK.cmake and OptionsWPE.cmake and reuse it here. > Source/WebCore/CMakeLists.txt:4103 > + COTIRE_PREFIX_HEADER_INCLUDE_PATH "${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesGTK/Root;${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesWPE/Root" Ditto. > Source/WebCore/WebCorePrefix.h:129 > +#elif PLATFORM(GTK) || PLATFORM(WPE) WebCorePrefix.h seems like the perfect location for this. I'm not sure why you think it might not be OK. (In reply to Michael Catanzaro from comment #5) > Comment on attachment 319615 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319615&action=review > > Awesome! I did not realize precompiled headers existed on Linux. > > The necessity for the CCACHE_SLOPPINESS environment variable is the main > blocker here. It has to work without needing any environment variables. > ccache gets pulled in with the @developer-tools group on Fedora so it's > expected to be present, but developers should not be expected to know what > it is. > I strongly disagree with this being a blocker. Really. Also, note that installing ccache doesn't mean you are using it. To actually use ccache you have to change your default PATH. $ which gcc /usr/lib/ccache/gcc And if you can change your PATH you can also change an environment variable. (In reply to Carlos Alberto Lopez Perez from comment #6) > (In reply to Michael Catanzaro from comment #5) > > Comment on attachment 319615 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=319615&action=review > > > > Awesome! I did not realize precompiled headers existed on Linux. > > > > The necessity for the CCACHE_SLOPPINESS environment variable is the main > > blocker here. It has to work without needing any environment variables. > > ccache gets pulled in with the @developer-tools group on Fedora so it's > > expected to be present, but developers should not be expected to know what > > it is. > > > > I strongly disagree with this being a blocker. Really. > > Also, note that installing ccache doesn't mean you are using it. > > To actually use ccache you have to change your default PATH. > > $ which gcc > /usr/lib/ccache/gcc > > And if you can change your PATH you can also change an environment variable. In any case the script build-webkit maybe can set this environment variable before executing CMake. That way everybody gets the right env var if they are using CMake. (In reply to Carlos Alberto Lopez Perez from comment #6) > (In reply to Michael Catanzaro from comment #5) > > Comment on attachment 319615 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=319615&action=review > > > > Awesome! I did not realize precompiled headers existed on Linux. > > > > The necessity for the CCACHE_SLOPPINESS environment variable is the main > > blocker here. It has to work without needing any environment variables. > > ccache gets pulled in with the @developer-tools group on Fedora so it's > > expected to be present, but developers should not be expected to know what > > it is. > > > > I strongly disagree with this being a blocker. Really. Totally not a blocker. If you use CCache and do not set the environment variable, compilation completes successfully anyway — CCache will just have cache misses, as explained in the manual: https://ccache.samba.org/manual.html#_precompiled_headers > Also, note that installing ccache doesn't mean you are using it. > > To actually use ccache you have to change your default PATH. > > $ which gcc > /usr/lib/ccache/gcc > > And if you can change your PATH you can also change an environment variable. Yep. CCache usage is optional, and it the user making the build is responsible to opt-in to use it. The same goes for IceCC. It has always been like that. (In reply to Konstantin Tokarev from comment #4) > I think we should use https://github.com/larsch/cmake-precompiled-header for > PCH and static *AllInOne.cpp files for unity builds. Sorry, but cmake-precompiled-header does not support Clang — Cotire does. That being said, if there is still a strong preference against Cotire, I can try and modify cmake-precompiled-header to add Clang support. At any rate, I think not supporting Clang for precompiled headers would be bad, provided that some developers prefer it due to the (sometimes) better error diagnostics and slightly faster build times. Also, some GNU/Linux distributions which have old versions of GCC as the system compiler are using Clang to build WebKitGTK+ and it would be nice that they could also get faster builds by allowing usage of precompiled headers with Clang. (In reply to Michael Catanzaro from comment #5) > Comment on attachment 319615 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319615&action=review > > Awesome! I did not realize precompiled headers existed on Linux. > > The necessity for the CCACHE_SLOPPINESS environment variable is the main > blocker here. It has to work without needing any environment variables. > ccache gets pulled in with the @developer-tools group on Fedora so it's > expected to be present, but developers should not be expected to know what > it is. > > > Source/JavaScriptCore/CMakeLists.txt:1607 > > +if (COMMAND cotire) > > How does this conditional work? It's testing for what exactly? This checks whether a CMake function named “cotire” is available. When Cotire is not loaded, this evaluates to “false” (note that CMake is confusing: it uses the term “command” for functions that can be used in CMake files). > > Source/JavaScriptCore/CMakeLists.txt:1611 > > + COTIRE_PREFIX_HEADER_INCLUDE_PATH "${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesGTK/Root;${CMAKE_SOURCE_DIR}/WebKitBuild/DependenciesWPE/Root" > > r- also for the hardcoded paths. This is at least easy to fix; you can > define some CMake variable differently in OptionsGTK.cmake and > OptionsWPE.cmake and reuse it here. Un-hardcoding these is a good suggestion, thanks! > > Source/WebCore/WebCorePrefix.h:129 > > +#elif PLATFORM(GTK) || PLATFORM(WPE) > > WebCorePrefix.h seems like the perfect location for this. I'm not sure why > you think it might not be OK. I also think that the existing prefix headers look like the perfect place for this, but as I had never touched them before and the GTK+/WPE ports didn't use them before, I was not 100% sure :-) So I am leaving a fresh build overnight in my laptop with the PCH support added for the WebKit+WebProcess+NetworkProcess targets, and if all goes well (as I expect!) tomorrow I'll post an updated patch and new figures for the build times. (In reply to Carlos Alberto Lopez Perez from comment #6) > To actually use ccache you have to change your default PATH. > > $ which gcc > /usr/lib/ccache/gcc > > And if you can change your PATH you can also change an environment variable. The assumption that the user has intentionally enabled ccache, or even knows what ccache is, is not correct. For instance, on my machine: $ which gcc /usr/lib64/ccache/gcc I didn't add that to my path, or do anything at all to configure ccache. Fedora has /etc/profile.d/ccache.sh to ensure it gets enabled without you having to know that ccache exists. It's not installed by default, but I did not intentionally install it either. At some point I ran the command 'dnf install @c-development' which pulled it in. Safe to assume many, many developers will have done that. (In reply to Carlos Alberto Lopez Perez from comment #7) > In any case the script build-webkit maybe can set this environment variable > before executing CMake. That way everybody gets the right env var if they > are using CMake. That's not good enough. We need to set it from the CMake build system. Fortunately that should be very easy. It can be done with something like set(ENV{CCACHE_SLOPPINESS} ...). Let's just add that somewhere and move on. (In reply to Adrian Perez from comment #8) > Totally not a blocker. If you use CCache and do not set the environment > variable, compilation completes successfully anyway — CCache will just > have cache misses, as explained in the manual: > > https://ccache.samba.org/manual.html#_precompiled_headers You said there would be a warning, and we don't allow build warnings. Anyway, we're lucky that it's easy enough to avoid. (In reply to Konstantin Tokarev from comment #4) > I think we should use https://github.com/larsch/cmake-precompiled-header for > PCH and static *AllInOne.cpp files for unity builds. Why is that better than Cotire? (In reply to Michael Catanzaro from comment #12) > [...] > > You said there would be a warning, and we don't allow build warnings. > Anyway, we're lucky that it's easy enough to avoid. It's not a warning while compilation is in progress. CMake prints a message during configuration telling the user (that is: *warning* the user) that the environment variable should be defined later on while building. Ah, OK then. I think that's OK. It's not so bad. I guess it's not so easy to get the environment variable set after all. :( Created attachment 319687 [details]
Alternative take, reusing existing macro
It turns out, the WebKit CMake files already had support
for doing precompiled headers with MSVC, so the best solution
in the end seemed to be to just hook there the missing CMake
spell to have it working for GCC and Clang. I reached this
solution after a long night trying to understand Cotire and
the suggestion by Konstantin, cmake-precompiled-header. Right
now I left my laptop doing a build, so no timings yet until
I get some hours of sleep -- hopefully the build will complete.
FWIW, personally I prefer this much simpler approach :-D
P.S: The patch will not pass the style checker, it is quite WIP
but I wanted to share it anyway so you can all give feedback.
(In reply to Adrian Perez from comment #16) > [...] > > P.S: The patch will not pass the style checker, it is quite WIP > but I wanted to share it anyway so you can all give feedback. Also the ChangeLog is not properly formatted O:-) Attachment 319687 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5]
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:1: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 2 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Well, the build didn't finish (see log below), but the method from the simple patch mostly works: the PCH for JavaScriptCore got generated and used, but then for WebCore it failed because there must be some “-I...” flag missing when invoking the compiler to build the PCH. Maybe I'll have some time later today to figure out how to pull the missing compiler flags. (BTW, the result is the same with Clang, so the switcheroo for chosing the .gch/.pch suffix depending on the compiler is working well, too!) --- Build Output --- [168/6006] Generating ../../DerivedSources/JavaScriptCore/LLIntDesiredOffsets.h Only dealing with backends: ["X86_64"] [1527/6006] Compiling PCH /home/aperez/devel/WebKit/WebKitBuild/Release/Source/WebCore/WebCoreDerivedSourcesPrefix.h.gch FAILED: Source/WebCore/WebCoreDerivedSourcesPrefix.h.gch cd /home/aperez/devel/WebKit/WebKitBuild/Release/Source/WebCore && /usr/bin/g++ -x c++-header -o /home/aperez/devel/WebKit/WebKitBuild/Release/Source/WebCore/WebCoreDerivedSourcesPrefix.h.gch /home/aperez/devel/WebKit/Source/WebCore/WebCoreDerivedSourcesPrefix.cpp In file included from /home/aperez/devel/WebKit/Source/WebCore/WebCoreDerivedSourcesPrefix.cpp:26:0: /home/aperez/devel/WebKit/Source/WebCore/WebCorePrefix.h:34:10: fatal error: wtf/Platform.h: No such file or directory #include <wtf/Platform.h> ^~~~~~~~~~~~~~~~ compilation terminated. Created attachment 319715 [details]
Simple approach, now with include paths and defines passed when building PCHs
Here goes a new version of the simpler approach, which is not so
simple anymore, but it does not properly add the compiler defines
and include paths when compiling the prefix headers. In order to
get the lists of flags an extra parameter is passed to the macro,
which is the name of the target from which to pick the compiler
flags. The flags are written to a response file because that way
it is done as the last step after all the compilation setup has
been completed -- otherwise not all the flags get picked, in case
the WEBKIT_ADD_PRECOMPILED_HEADER is called *before* additional
options are set, which can happen in port-specific CMake snippets.
This version properly builds the PCHs, and I am waiting for the
builds to finish to be able to provide timings and validate that
WebKit is built correctly.
:-D
Attachment 319715 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5]
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:1: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 2 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 319716 [details]
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail.
Attachment 319716 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5]
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:1: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 2 in 14 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 319716 [details] WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail. View in context: https://bugs.webkit.org/attachment.cgi?id=319716&action=review > Source/WebKit/WebKit2Prefix.h:75 > +#if USE(LIBEPOXY) > +#include <epoxy.h> > +#endif We need to remove this from WebKit2Prefix.h because it's not in the include path. Seems this layer does not use libepoxy. See the build failure on the WPE bot. > Source/cmake/WebKitMacros.cmake:99 > + add_custom_command( > + OUTPUT "${PrecompiledBinary}" > + MAIN_DEPENDENCY "${_cpp}" > + DEPENDS "${PrecompiledBinary}.flags" > + COMMENT "Compiling PCH ${PrecompiledBinary}" > + COMMAND "${CMAKE_CXX_COMPILER}" "@${PrecompiledBinary}.flags" > + -x c++-header ${_pch_cxx_flags} -o "${PrecompiledBinary}" > + "${CMAKE_CURRENT_SOURCE_DIR}/${_cpp}") So we did not need cotire after all? The bug title and ChangeLog entries need to be updated with the newer name. > ChangeLog:16 > +2017-09-01 Adrian Perez de Castro <aperez@igalia.com> > + > + [CMake] Enable pre-compiled headers (PCH) (cotire) > + https://bugs.webkit.org/show_bug.cgi?id=139438 > + > + Reviewed by NOBODY (OOPS!). > + > + * Source/cmake/WebKitMacros.cmake: > + > +2017-09-01 Adrian Perez de Castro <aperez@igalia.com> > + > + [CMake] Enable pre-compiled headers (PCH) (cotire) > + https://bugs.webkit.org/show_bug.cgi?id=139438 > + > + Reviewed by NOBODY (OOPS!). > + Duplicate ChangeLog entry. (In reply to Michael Catanzaro from comment #24) > Comment on attachment 319716 [details] > WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=319716&action=review > > > Source/WebKit/WebKit2Prefix.h:75 > > +#if USE(LIBEPOXY) > > +#include <epoxy.h> > > +#endif > > We need to remove this from WebKit2Prefix.h because it's not in the include > path. Seems this layer does not use libepoxy. See the build failure on the > WPE bot. Good catch. I have not tried building WPE yet, so this was an speculative addition to the precompiled header. Once we have the initial support landed we can look at what gets often included and try to add more #includes in the prefix headers to further improve build times — IMHO that would be fine as a follow-up patch :-) > > Source/cmake/WebKitMacros.cmake:99 > > + add_custom_command( > > + OUTPUT "${PrecompiledBinary}" > > + MAIN_DEPENDENCY "${_cpp}" > > + DEPENDS "${PrecompiledBinary}.flags" > > + COMMENT "Compiling PCH ${PrecompiledBinary}" > > + COMMAND "${CMAKE_CXX_COMPILER}" "@${PrecompiledBinary}.flags" > > + -x c++-header ${_pch_cxx_flags} -o "${PrecompiledBinary}" > > + "${CMAKE_CURRENT_SOURCE_DIR}/${_cpp}") > > So we did not need cotire after all? The bug title and ChangeLog entries > need to be updated with the newer name. Noup, the simple approach works fine, thanks to the WebKit source code being well organized and the usage we make of CMake being consistent. Cotire does *a lot* of things because it tries to work in all possible situations (old CMake versions, mixed C/C++ projects, compilers other than GCC/Clang/MSVC, etc), and also it is made in such a way that it queries *existing, already configured* CMake targets to allow adding PCH+unitybuild support for any existing project, which is *very* hard (I looked at the code, it's scaaary!); therefore it has a lot of code needed to support all those scenarios. > > ChangeLog:16 > > +2017-09-01 Adrian Perez de Castro <aperez@igalia.com> > > + > > + [CMake] Enable pre-compiled headers (PCH) (cotire) > > + https://bugs.webkit.org/show_bug.cgi?id=139438 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > + * Source/cmake/WebKitMacros.cmake: > > + > > +2017-09-01 Adrian Perez de Castro <aperez@igalia.com> > > + > > + [CMake] Enable pre-compiled headers (PCH) (cotire) > > + https://bugs.webkit.org/show_bug.cgi?id=139438 > > + > > + Reviewed by NOBODY (OOPS!). > > + > > Duplicate ChangeLog entry. Yeah, I have been updating the attached patch to this bug often, to let people see how it is coming along. It will need at least one last cleanup pass (I have a couple of ideas to simplify macro invocations) and fixing the ChangeLogs and other style violations. At any rate, I will be updating with updates along the weekend — I am determined to have PCH support working ASAP, because it will benefit infrastructure (faster buildbots), developer experience (shorter edit-compile-test cycle), and distributors (shorter build times to update packages). Comment on attachment 319716 [details]
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail.
I don't see anything major wrong with this, but you have some comments to address so let's get this out of the request queue for now.
Created attachment 320349 [details]
Patch
Updated version. Now this picks all the relevant
build flags which affect generation of the PCHs. After much back and forth
reading the CMake documentation, and checking what Cotire does (it turns
out it does many things because it *does* need to do them) I came up with
a not-so-big as Cotire yet not-as-simple as the solution pointed out by
Konstantin (which didn not work becayse it was missing flags).
Still pending to do actual full builds, with this I am getting consistently
between 1 and 2 minutes of saved build time when building only JSC. Also
gotta add the ChageLog entries and all -- but again, sharing early for
feedback. If somebody could run with this patch on MSVC to make sure I
did not break anything, that would be swell.
Attachment 320349 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 1 in 10 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Adrian Perez from comment #27) > [...] If somebody could run with this patch on MSVC to make sure I > did not break anything, that would be swell. ...aaand of course the somebody is the Windows EWS. I'll fix it, and also submit a version of the patch that enables PCHs by default, so all the EWS run through it enabled. Created attachment 320359 [details]
Patch (not for landing)
This is an updated version with PCHs always enabled in order to
test things in all the EWS bots. If this works I will re-upload with the
check for ENABLE_PRECOMPILED_HEADERS in.
Attachment 320359 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/JavaScriptCorePrefix.cpp:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 320359 [details] Patch (not for landing) The patch looks good. I'm willing to do some benchmarks with it once it works ok. For now, setting r- due to red EWS. I tried it also locally and failed to build with: FAILED: Source/WebCore/WebCore_DERIVED_SOURCES/pch.h.gch cd /home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore && /usr/bin/c++ @/home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore/WebCore_DERIVED_SOURCES/flags -x c++-header -o /home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore/WebCore_DERIVED_SOURCES/pch.h.gch /home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore/WebCore_DERIVED_SOURCES/pch.h In file included from /usr/include/c++/6/bits/stl_algo.h:59:0, from /usr/include/c++/6/algorithm:62, from /home/clopez/webkit/webkit/Source/WebCore/WebCorePrefix.h:81, from /home/clopez/webkit/webkit/WebKitBuild/Release/Source/WebCore/WebCore_DERIVED_SOURCES/pch.h:1: /usr/include/c++/6/cstdlib:75:25: fatal error: stdlib.h: No such file or directory #include_next <stdlib.h> ^ Which reminds me to bug 161697 (maybe related, take a look) We probably want to use the built-in support for precompiled headers which will be available starting with CMake 3.16: https://onqtam.com/programming/2019-12-20-pch-unity-cmake-3-16/ Maybe it would be possible to have a macro which actually calls CMake's target_precompile_headers() when using CMake 3.16 or newer, and which is a no-op otherwise (for older versions). We could also add cmake to jhbuild so it would be easier for developers to get this feature I did some experiments with this feature and I'd like to warn you that 3.16 is buggy, please use 3.17 at least Created attachment 441574 [details]
WIP Patch
New version using the CMake builtin PCH support
Created attachment 441576 [details]
WIP Patch v2
Created attachment 441585 [details]
WIP Patch v3
Created attachment 441593 [details]
WIP Patch v4
Created attachment 441627 [details]
WIP Patch v5
Created attachment 441628 [details]
WIP Patch v6
Created attachment 441636 [details]
WIP Patch v7
Created attachment 441649 [details]
WIP Patch v8
Created attachment 441769 [details]
WIP Patch v9
Comment on attachment 441769 [details] WIP Patch v9 View in context: https://bugs.webkit.org/attachment.cgi?id=441769&action=review Looks like you're super close. > Source/WebKitLegacy/CMakeLists.txt:50 > +WEBKIT_ADD_PRECOMPILED_HEADER(WebKitLegacy WebKitPrefix.h) This should be done in the PlatformWin.cmake and the header is win/WebkitPrefix.h For PlatformMac it would be mac/WebKitPrefix.h > Tools/WebKitTestRunner/PlatformFTW.cmake:-32 > -WEBKIT_ADD_PRECOMPILED_HEADER("WebKitTestRunnerPrefix.h" "win/WebKitTestRunnerPrefix.cpp" WebKitTestRunner_SOURCES) This again looks like you're going to hit an issue due to the location of the prefix header. (In reply to Don Olmstead from comment #45) > Comment on attachment 441769 [details] > WIP Patch v9 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=441769&action=review > > Looks like you're super close. > > > Source/WebKitLegacy/CMakeLists.txt:50 > > +WEBKIT_ADD_PRECOMPILED_HEADER(WebKitLegacy WebKitPrefix.h) > > This should be done in the PlatformWin.cmake and the header is > win/WebkitPrefix.h For PlatformMac it would be mac/WebKitPrefix.h > > > Tools/WebKitTestRunner/PlatformFTW.cmake:-32 > > -WEBKIT_ADD_PRECOMPILED_HEADER("WebKitTestRunnerPrefix.h" "win/WebKitTestRunnerPrefix.cpp" WebKitTestRunner_SOURCES) > > This again looks like you're going to hit an issue due to the location of > the prefix header. This souldn't be an issue, the header is indeed located at “Tools/WebKitTestRunner/WebKitTestRunnerPrefix.h” and with the CMake PCH support the .cpp file is not really needed. (Actually, next version of the patch will remove the unneeded files as well.) Created attachment 441792 [details]
WIP Patch v10
``` C:\cygwin\home\buildbot\worker\Windows-EWS\build\Source\WebCore\platform\graphics\cg\ImageBufferCGBitmapBackend.cpp(146,36): error C2672: 'WTF::adoptCF': no matching overloaded function found [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj] ``` This AppleWin failure is most likely due to WebCorePrefixHeader.h including CoreFoundation files so its a legit failure. Created attachment 441957 [details]
Windows fixes
The DumpRenderTree and WebKitTestRunner bits on Windows use WEBKIT_EXECUTABLE_WRAP which creates a DLL that has all the code that the executable then runs. Apply to that wrapper library to prevent the build error seen on the bots.
Windows is happy. Not sure why WPE decided to stop building. Anyways I'll leave the rest to you Adrian. (In reply to Don Olmstead from comment #50) > Windows is happy. Not sure why WPE decided to stop building. > > Anyways I'll leave the rest to you Adrian. Great, thanks for your support! I will integrate your fixes, check why the WPE builder is choking now on the patch, and make sure this gets landed in the next days \o/ Created attachment 442060 [details]
Windows and PlayStation fixes
Comment on attachment 442060 [details] Windows and PlayStation fixes View in context: https://bugs.webkit.org/attachment.cgi?id=442060&action=review Additional changes were required for a local PlayStation build so please use this as your base. > Source/WebCore/testing/js/WebCoreTestSupportPrefix.h:-48 > - > -#define WEBCORE_TESTSUPPORT_EXPORT WEBCORE_EXPORT > - This should be properly handled in PlatformExportMacros.h 🤞 > Source/WebCore/testing/js/WebCoreTestSupportPrefix.h:130 > +#elif defined(__APPLE__) > + > #if !PLATFORM(IOS_FAMILY) > #include <CoreServices/CoreServices.h> > #endif // !PLATFORM(IOS_FAMILY) PlayStation was ending up trying to include CoreServices.h so changed this into one big #if #elif setup to prevent that. I left a script running overnight undisturbed in an old laptop, which did three builds without this patch, and three builds after applying it. The builds were done as of r284652 with the Flatpak SDK using the following command: WK_USE_CCACHE=NO ./Tools/Scripts/build-webkit --gtk --no-experimental-features The averaged build times were: Before: 2h:22m:07s After: 2h:12m:54s While not much faster, it's still a ~7% speedup, and we get to simplify and unify the handling of precompiled headers. We can probably tweak the prefix headers to make a few more big #includes to further improve things, but I would rather do that in a follow-up =) Well, I don't know why the linker is being passed the precompiled header or why that's an issue, but at least now I can reproduce it locally: doing a build with the patch applied on top of a dirty build directory using the Flatpak SDK hits it. Investigating… Created attachment 442431 [details]
WIP Patch v11
Created attachment 442432 [details]
WIP Patch v12
I have now a version of the patch that also makes the WPE port build correctly. Just one pass more to clean up a couple of things and I will post the finished patch to land :-) (In reply to Adrian Perez from comment #58) > I have now a version of the patch that also makes the WPE port build > correctly. Just one pass more to clean up a couple of things and I will > post the finished patch to land :-) Wait! The plot thickens: % cmake --version | head -1 cmake version 3.21.4 % cmake --version | head -1 cmake version 3.21.2 % System CMake (the newer one) works, but the one from the Flatpak SDK still doesn't. Checking the diff between both versions I cannot find anything obvious that would affect how target_precompile_headers() works. (In reply to Adrian Perez from comment #59) > (In reply to Adrian Perez from comment #58) > > I have now a version of the patch that also makes the WPE port build > > correctly. Just one pass more to clean up a couple of things and I will > > post the finished patch to land :-) > > Wait! The plot thickens: > > % cmake --version | head -1 > cmake version 3.21.4 > % cmake --version | head -1 > cmake version 3.21.2 > % > > System CMake (the newer one) works, but the one from the Flatpak SDK still > doesn't. Checking the diff between both versions I cannot find anything > obvious that would affect how target_precompile_headers() works. Ah, also my non-Flatpak build dir is configured to use Clang; I have to try with a configuration more similar to what “build-webkit“ does. That being said... I am tempted to disable PCH for the JavaScriptCore target for the WPE port and try to fix it after this patch lands. Opionions? (In reply to Adrian Perez from comment #60) > (In reply to Adrian Perez from comment #59) > > (In reply to Adrian Perez from comment #58) > > > I have now a version of the patch that also makes the WPE port build > > > correctly. Just one pass more to clean up a couple of things and I will > > > post the finished patch to land :-) > > > > Wait! The plot thickens: > > > > % cmake --version | head -1 > > cmake version 3.21.4 > > % cmake --version | head -1 > > cmake version 3.21.2 > > % > > > > System CMake (the newer one) works, but the one from the Flatpak SDK still > > doesn't. Checking the diff between both versions I cannot find anything > > obvious that would affect how target_precompile_headers() works. > > Ah, also my non-Flatpak build dir is configured to use Clang; I have to > try with a configuration more similar to what “build-webkit“ does. Further narrowing down: This happens only when the compiler is GCC. Aither CMake generates correct build rules when Clang is the compiler, or Clang is ignoring passed PCH files at link time. (In reply to Adrian Perez from comment #61) > (In reply to Adrian Perez from comment #60) > > (In reply to Adrian Perez from comment #59) > > > (In reply to Adrian Perez from comment #58) > > > > I have now a version of the patch that also makes the WPE port build > > > > correctly. Just one pass more to clean up a couple of things and I will > > > > post the finished patch to land :-) > > > > > > Wait! The plot thickens: > > > > > > % cmake --version | head -1 > > > cmake version 3.21.4 > > > % cmake --version | head -1 > > > cmake version 3.21.2 > > > % > > > > > > System CMake (the newer one) works, but the one from the Flatpak SDK still > > > doesn't. Checking the diff between both versions I cannot find anything > > > obvious that would affect how target_precompile_headers() works. > > > > Ah, also my non-Flatpak build dir is configured to use Clang; I have to > > try with a configuration more similar to what “build-webkit“ does. > > Further narrowing down: This happens only when the compiler is GCC. Aither > CMake generates correct build rules when Clang is the compiler, or Clang is > ignoring passed PCH files at link time. Another round of testing later... - Both Clang and GCC trip with the .[pg]ch files being added in the linker command line. - Clang with *LLD* does not trip, no idea if it's LLD ignoring the file or the compiler driver removing it from the linker command. - In any case, the build.ninja/Makefile files produced by CMake have an invalid command line. - Seems to be a CMake bug, in the end. Don has found https://gitlab.kitware.com/cmake/cmake/-/issues/22630 which seems related but I built a patched CMake 3.21.3 with the associated fix included and it did not help. At any rate, that is where I got an idea to avoid adding calling target_precompile_headers() when the target is of type OBJECT_LIBRARY — this works. Created attachment 443247 [details]
WIP Patch v13
Current version, for the sake of running it through the EWS.
Created attachment 447615 [details]
Patch v14
Rebased, let's try and sort out the Windows build issues
Comment on attachment 447615 [details] Patch v14 View in context: https://bugs.webkit.org/attachment.cgi?id=447615&action=review > Source/cmake/WebKitMacros.cmake:102 > + if (NOT type STREQUAL OBJECT_LIBRARY) This patch doesn't work as expected. type should be ${type} > if (NOT ${type} STREQUAL OBJECT_LIBRARY) Comment on attachment 447615 [details] Patch v14 View in context: https://bugs.webkit.org/attachment.cgi?id=447615&action=review >> Source/cmake/WebKitMacros.cmake:102 >> + if (NOT type STREQUAL OBJECT_LIBRARY) > > This patch doesn't work as expected. type should be ${type} This condition is unfortunate for WinCairo port. WinCairo is using OBJECT library for WebCore. Comment on attachment 447615 [details] Patch v14 View in context: https://bugs.webkit.org/attachment.cgi?id=447615&action=review > Source/cmake/WebKitMacros.cmake:99 > +if (ENABLE_PRECOMPILED_HEADERS) ENABLE_PRECOMPILED_HEADERS isn't defined yet in the first run. I think you should move the condition into the function. function(WEBKIT_ADD_PRECOMPILED_HEADER _target _header) if (ENABLE_PRECOMPILED_HEADERS) ... endif () endfunction() (In reply to Fujii Hironori from comment #65) > Comment on attachment 447615 [details] > Patch v14 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447615&action=review > > > Source/cmake/WebKitMacros.cmake:102 > > + if (NOT type STREQUAL OBJECT_LIBRARY) > > This patch doesn't work as expected. type should be ${type} > > > if (NOT ${type} STREQUAL OBJECT_LIBRARY) This works fine as-is, CMake tries to expand variables even if ${} is not used to explicitly expand them inside “if” conditions (yeah, CMake is weird, I can use ${type} here for the sake of clarity, though!) (In reply to Fujii Hironori from comment #66) > Comment on attachment 447615 [details] > Patch v14 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447615&action=review > > >> Source/cmake/WebKitMacros.cmake:102 > >> + if (NOT type STREQUAL OBJECT_LIBRARY) > > > > This patch doesn't work as expected. type should be ${type} > > This condition is unfortunate for WinCairo port. > WinCairo is using OBJECT library for WebCore. We also use an object library for JavaScriptCore in the WPE port. I hope that CMake eventually fixes this bug. (In reply to Fujii Hironori from comment #67) > Comment on attachment 447615 [details] > Patch v14 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447615&action=review > > > Source/cmake/WebKitMacros.cmake:99 > > +if (ENABLE_PRECOMPILED_HEADERS) > > ENABLE_PRECOMPILED_HEADERS isn't defined yet in the first run. > I think you should move the condition into the function. > > function(WEBKIT_ADD_PRECOMPILED_HEADER _target _header) > if (ENABLE_PRECOMPILED_HEADERS) > ... > endif () > endfunction() Will do, thanks for the suggestion. Comment on attachment 447615 [details] Patch v14 View in context: https://bugs.webkit.org/attachment.cgi?id=447615&action=review >>>>> Source/cmake/WebKitMacros.cmake:102 >>>>> + if (NOT type STREQUAL OBJECT_LIBRARY) >>>> >>>> This patch doesn't work as expected. type should be ${type} >>> >>> This condition is unfortunate for WinCairo port. >>> WinCairo is using OBJECT library for WebCore. >> >> This works fine as-is, CMake tries to expand variables even if ${} >> is not used to explicitly expand them inside “if” conditions (yeah, >> CMake is weird, I can use ${type} here for the sake of clarity, though!) > > We also use an object library for JavaScriptCore in the WPE port. > I hope that CMake eventually fixes this bug. My bad. I tried it again locally. The condition works. Is it a good idea to quote a string? if (NOT type STREQUAL "OBJECT_LIBRARY") Comment on attachment 447615 [details] Patch v14 View in context: https://bugs.webkit.org/attachment.cgi?id=447615&action=review >>>>>> Source/cmake/WebKitMacros.cmake:102 >>>>>> + if (NOT type STREQUAL OBJECT_LIBRARY) >>>>> >>>>> This patch doesn't work as expected. type should be ${type} >>>> >>>> This condition is unfortunate for WinCairo port. >>>> WinCairo is using OBJECT library for WebCore. >>> >>> This works fine as-is, CMake tries to expand variables even if ${} >>> is not used to explicitly expand them inside “if” conditions (yeah, >>> CMake is weird, I can use ${type} here for the sake of clarity, though!) >> >> We also use an object library for JavaScriptCore in the WPE port. >> I hope that CMake eventually fixes this bug. > > My bad. I tried it again locally. The condition works. Is it a good idea to quote a string? > if (NOT type STREQUAL "OBJECT_LIBRARY") As far as I tested locally, target_precompile_headers works for OBJECT libraries in WinCairo port. Can you specify more conditions for the workaround? (In reply to Fujii Hironori from comment #72) > Comment on attachment 447615 [details] > Patch v14 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447615&action=review > > >>>>>> Source/cmake/WebKitMacros.cmake:102 > >>>>>> + if (NOT type STREQUAL OBJECT_LIBRARY) > >>>>> > >>>>> This patch doesn't work as expected. type should be ${type} > >>>> > >>>> This condition is unfortunate for WinCairo port. > >>>> WinCairo is using OBJECT library for WebCore. > >>> > >>> This works fine as-is, CMake tries to expand variables even if ${} > >>> is not used to explicitly expand them inside “if” conditions (yeah, > >>> CMake is weird, I can use ${type} here for the sake of clarity, though!) > >> > >> We also use an object library for JavaScriptCore in the WPE port. > >> I hope that CMake eventually fixes this bug. > > > > My bad. I tried it again locally. The condition works. Is it a good idea to quote a string? > > if (NOT type STREQUAL "OBJECT_LIBRARY") > > As far as I tested locally, target_precompile_headers works for OBJECT > libraries in WinCairo port. > Can you specify more conditions for the workaround? Sure, we could try and enable the workaround only for non-MSVC compilers. (Also I need to rebase the patch and update it a bit.) 257057@main (bug#248202) landed the most part of your patch for Windows ports. CMAKE_DISABLE_PRECOMPILE_HEADERS should be OFF for all ports. |