Bug 139438 - [CMake] Enable pre-compiled headers (PCH)
Summary: [CMake] Enable pre-compiled headers (PCH)
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adrian Perez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-09 02:43 PST by Carlos Alberto Lopez Perez
Modified: 2017-09-21 00:10 PDT (History)
13 users (show)

See Also:


Attachments
WIP patch. Enables pre-compiled headers on WTF and JSC. Fails on WebCore (commented out) (156.65 KB, patch)
2014-12-09 03:13 PST, Carlos Alberto Lopez Perez
no flags Details | Formatted Diff | Diff
Patch (175.25 KB, patch)
2017-09-01 10:47 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Alternative take, reusing existing macro (10.97 KB, patch)
2017-09-01 20:30 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Simple approach, now with include paths and defines passed when building PCHs (14.42 KB, patch)
2017-09-02 07:15 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail. (14.41 KB, patch)
2017-09-02 08:55 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (19.51 KB, patch)
2017-09-09 10:15 PDT, Adrian Perez
no flags Details | Formatted Diff | Diff
Patch (not for landing) (29.13 KB, patch)
2017-09-09 14:47 PDT, Adrian Perez
clopez: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Alberto Lopez Perez 2014-12-09 02:43:45 PST
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
Comment 1 Carlos Alberto Lopez Perez 2014-12-09 03:13:43 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.
Comment 2 Adrian Perez 2017-09-01 10:47:28 PDT
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.)
Comment 3 Adrian Perez 2017-09-01 10:52:48 PDT
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.
Comment 4 Konstantin Tokarev 2017-09-01 11:32:02 PDT
I think we should use https://github.com/larsch/cmake-precompiled-header for PCH and static *AllInOne.cpp files for unity builds.
Comment 5 Michael Catanzaro 2017-09-01 11:32:19 PDT
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.
Comment 6 Carlos Alberto Lopez Perez 2017-09-01 11:47:15 PDT
(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.
Comment 7 Carlos Alberto Lopez Perez 2017-09-01 11:54:50 PDT
(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.
Comment 8 Adrian Perez 2017-09-01 13:03:43 PDT
(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.
Comment 9 Adrian Perez 2017-09-01 13:10:08 PDT
(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.
Comment 10 Adrian Perez 2017-09-01 13:14:49 PDT
(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 :-)
Comment 11 Adrian Perez 2017-09-01 13:25:44 PDT
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.
Comment 12 Michael Catanzaro 2017-09-01 13:28:08 PDT
(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.
Comment 13 Carlos Alberto Lopez Perez 2017-09-01 13:46:05 PDT
(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?
Comment 14 Adrian Perez 2017-09-01 13:49:48 PDT
(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.
Comment 15 Michael Catanzaro 2017-09-01 14:02:12 PDT
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. :(
Comment 16 Adrian Perez 2017-09-01 20:30:03 PDT
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.
Comment 17 Adrian Perez 2017-09-01 20:30:52 PDT
(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:-)
Comment 18 Build Bot 2017-09-01 20:31:41 PDT
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.
Comment 19 Adrian Perez 2017-09-02 04:08:25 PDT
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.
Comment 20 Adrian Perez 2017-09-02 07:15:03 PDT
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
Comment 21 Build Bot 2017-09-02 07:16:42 PDT
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.
Comment 22 Adrian Perez 2017-09-02 08:55:56 PDT
Created attachment 319716 [details]
WIP, fixes WebCoreDerivedSourcesPrefix; the style checker will still fail.
Comment 23 Build Bot 2017-09-02 08:57:48 PDT
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 24 Michael Catanzaro 2017-09-02 10:31:09 PDT
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.
Comment 25 Adrian Perez 2017-09-02 11:14:29 PDT
(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 26 Michael Catanzaro 2017-09-05 18:05:12 PDT
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.
Comment 27 Adrian Perez 2017-09-09 10:15:38 PDT
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.
Comment 28 Build Bot 2017-09-09 10:17:26 PDT
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.
Comment 29 Adrian Perez 2017-09-09 14:26:48 PDT
(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.
Comment 30 Adrian Perez 2017-09-09 14:47:00 PDT
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.
Comment 31 Build Bot 2017-09-09 14:49:59 PDT
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 32 Carlos Alberto Lopez Perez 2017-09-14 06:10:18 PDT
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)