[CMake] Enable pre-compiled headers (PCH)

[Carlos Alberto Lopez Perez]

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

[Carlos Alberto Lopez Perez]

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.

[Adrian Perez]

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.)

[Adrian Perez]

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.

[Konstantin Tokarev]

I think we should use https://github.com/larsch/cmake-precompiled-header for PCH and static *AllInOne.cpp files for unity builds.

[Michael Catanzaro]

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.

[Carlos Alberto Lopez Perez]

(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.

[Carlos Alberto Lopez Perez]

(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.

[Adrian Perez]

(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.

[Adrian Perez]

(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.

[Adrian Perez]

(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 :-)

[Adrian Perez]

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.

[Michael Catanzaro]

(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.

[Carlos Alberto Lopez Perez]

(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?

[Adrian Perez]

(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.

[Michael Catanzaro]

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. :(

[Adrian Perez]

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.

[Adrian Perez]

(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:-)

[Build Bot]

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.

[Adrian Perez]

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.

[Adrian Perez]

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

[Build Bot]

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.

[Adrian Perez]

Created attachment 319716 [details]
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail.

[Build Bot]

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.

[Michael Catanzaro]

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
> [phone]  Adrian Perez de Castro  <[e-mail]>
> +
> +        [CMake] Enable pre-compiled headers (PCH) (cotire)
> +        https://bugs.webkit.org/show_bug.cgi?id=139438
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * Source/cmake/WebKitMacros.cmake:
> +
> [phone]  Adrian Perez de Castro  <[e-mail]>
> +
> +        [CMake] Enable pre-compiled headers (PCH) (cotire)
> +        https://bugs.webkit.org/show_bug.cgi?id=139438
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Duplicate ChangeLog entry.

[Adrian Perez]

(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
> > [phone]  Adrian Perez de Castro  <[e-mail]>
> > +
> > +        [CMake] Enable pre-compiled headers (PCH) (cotire)
> > +        https://bugs.webkit.org/show_bug.cgi?id=139438
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        * Source/cmake/WebKitMacros.cmake:
> > +
> > [phone]  Adrian Perez de Castro  <[e-mail]>
> > +
> > +        [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).

[Michael Catanzaro]

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.

[Adrian Perez]

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.

[Build Bot]

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.

[Adrian Perez]

(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.

[Adrian Perez]

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.

[Build Bot]

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.

[Carlos Alberto Lopez Perez]

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)

[Adrian Perez]

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).

[Konstantin Tokarev]

We could also add cmake to jhbuild so it would be easier for developers to get this feature

[Konstantin Tokarev]

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

[Adrian Perez]

Created attachment 441574 [details]
WIP Patch

New version using the CMake builtin PCH support

[Adrian Perez]

Created attachment 441576 [details]
WIP Patch v2

[Adrian Perez]

Created attachment 441585 [details]
WIP Patch v3

[Adrian Perez]

Created attachment 441593 [details]
WIP Patch v4

[Adrian Perez]

Created attachment 441627 [details]
WIP Patch v5

[Adrian Perez]

Created attachment 441628 [details]
WIP Patch v6

[Adrian Perez]

Created attachment 441636 [details]
WIP Patch v7

[Adrian Perez]

Created attachment 441649 [details]
WIP Patch v8

[Adrian Perez]

Created attachment 441769 [details]
WIP Patch v9

[Don Olmstead]

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.

[Adrian Perez]

(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.)

[Adrian Perez]

Created attachment 441792 [details]
WIP Patch v10

[Don Olmstead]

```
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.

[Don Olmstead]

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.

[Don Olmstead]

Windows is happy. Not sure why WPE decided to stop building.

Anyways I'll leave the rest to you Adrian.

[Adrian Perez]

(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/

[Don Olmstead]

Created attachment 442060 [details]
Windows and PlayStation fixes

[Don Olmstead]

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.

[Adrian Perez]

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 =)

[Adrian Perez]

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…

[Adrian Perez]

Created attachment 442431 [details]
WIP Patch v11

[Adrian Perez]

Created attachment 442432 [details]
WIP Patch v12

[Adrian Perez]

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 :-)

[Adrian Perez]

(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.

[Adrian Perez]

(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?

[Adrian Perez]

(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.

[Adrian Perez]

(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.

[Adrian Perez]

Created attachment 443247 [details]
WIP Patch v13


Current version, for the sake of running it through the EWS.

[Adrian Perez]

Created attachment 447615 [details]
Patch v14

Rebased, let's try and sort out the Windows build issues

[Fujii Hironori]

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)

[Fujii Hironori]

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.

[Fujii Hironori]

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()

[Adrian Perez]

(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!)

[Adrian Perez]

(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.

[Adrian Perez]

(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.

[Fujii Hironori]

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")

[Fujii Hironori]

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?

[Adrian Perez]

(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.)

[Fujii Hironori]

257057@main (bug#248202) landed the most part of your patch for Windows ports.
CMAKE_DISABLE_PRECOMPILE_HEADERS should be OFF for all ports.