The usage of pre-compiled headers can reduce compilation times. It has brought to my attention the existence of the the following CMake module to enable them: https://github.com/sakra/cotire
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