RFC: build: support for pre-compiled headers. #31053

theuni · 2024-10-07T21:33:32Z

This cuts off roughly a third of my total build time, either with -j8 or -j24.

There are many different ways to go about adding pre-compiled header support with CMake. This is probably the simplest and most naive, but it has a substantial impact.

I used ClangBuildAnalyzer to measure the headers which took the most amount of time to parse while building the kernel library. I did this under the assumption that those are probably the most included files in general, and because they should never be affected by changing defines/flags in different compilation units.

CMake re-generates a new pch file for each target that includes it, so it's only worth using for libraries/binaries which compile a substantial list of cpp files. I eyeballed the libs/bins which I thought made sense.

I chose to add base_precompiled as PRIVATE everywhere so that it's clear exactly where they're being used.

Other approaches would involve keeping (or generating) lists of proper headers for each lib/bin, and potentially sharing them around when possible. The approach here (a monolithic interface lib) is unfortunately incompatible with sharing.

There's an additional (hackish) commit to support ccache + pch. Unfortunately this requires a rather new (4.8+) ccache in order to properly set the variables we need. With the required options turned on as documented by ccache, this combo works fine for me locally.

Overall, I think this is a reasonable approach because it pretty much just works with no fiddling. The main drawback is that over time the headers list will grow stale, so it'll require some attention now and then.

Results: (-DBUILD_UTIL_CHAINSTATE=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_BENCH=ON)

Before:
**** Time summary:
Compilation (1412 times):
Parsing (frontend): 2619.3 s
Codegen & opts (backend): 1553.3 s

Real make time: 6m20.894s

After:
**** Time summary:
Compilation (1422 times):
Parsing (frontend): 1375.4 s
Codegen & opts (backend): 1726.7 s

Real make time: 4m24.634s

pch + ccache 2nd run: 0m10.073s

DrahtBot · 2024-10-07T21:33:35Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31053.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	TheCharlatan, hebasto, laanwj, edilmedeiros, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30882 (wip: Split fuzz binary (take 2) by dergoegge)
#30861 (build: Improve ccache performance for different build directories by hebasto)
#30811 (build: Unify -logsourcelocations format by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

theuni · 2024-10-07T21:35:41Z

Ping @hebasto, @fanquake

Also @willcl-ark, you might be interested in this as you've been benchmarking build times.

TheCharlatan · 2024-10-07T21:41:03Z

Concept ACK

DrahtBot · 2024-10-07T22:50:50Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/31200880897}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

fanquake · 2024-10-08T14:28:26Z

MSAN failure was spurious, but not the tidy (https://cirrus-ci.com/task/4930868089716736):

[21:49:26.130] clang-tidy-18 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/validationinterface.cpp
[21:49:26.130] /ci_container_base/src/validationinterface.cpp:22:13: error: redundant 'RemovalReasonToString' declaration [readability-redundant-declaration,-warnings-as-errors]
[21:49:26.130]    22 | std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept;
[21:49:26.130]       | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[21:49:26.130] /ci_container_base/src/kernel/mempool_removal_reason.h:22:13: note: previously declared here
[21:49:26.130]    22 | std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept;
[21:49:26.130]       |             ^
[21:49:26.130] ^^^ ⚠️ Failure generated from clang-tidy
[21:49:26.130] + echo '^^^ ⚠️ Failure generated from clang-tidy'
[21:49:26.130] + false

theuni · 2024-10-08T15:22:38Z

Huh, I guess that happens because pch shuffles around the include order.

The redundant decl indeed seems broken to me. Will PR a quick fix.

theuni · 2024-10-08T20:41:06Z

Combining with #30911 produces even more of a speedup (with Make, ninja is about the same).

Config: -DCMAKE_BUILD_TYPE=Debug -DWITH_CCACHE=OFF -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_BENCH=ON
Using: make -j24

Master: 2m7.311s
This PR: 1m34.495s
This PR + #30911: 1m19.046s

Result: Completes in 62% of the time compared to master.

…-declaring RemovalReasonToString ca2e4ba refactor: include the proper header rather than forward-declaring RemovalReasonToString (Cory Fields) Pull request description: Trivial no-op fixup. This was pointed out by #31053, which causes the include order to be shuffled around: ``` [21:49:26.130] /ci_container_base/src/validationinterface.cpp:22:13: error: redundant 'RemovalReasonToString' declaration [readability-redundant-declaration,-warnings-as-errors] [21:49:26.130] 22 | std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept; [21:49:26.130] | ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [21:49:26.130] /ci_container_base/src/kernel/mempool_removal_reason.h:22:13: note: previously declared here [21:49:26.130] 22 | std::string RemovalReasonToString(const MemPoolRemovalReason& r) noexcept; [21:49:26.130] | ^ ``` I don't see any reason why the include shouldn't just be used. ACKs for top commit: maflcko: lgtm ACK ca2e4ba hebasto: ACK ca2e4ba, IWYU seems [agree](https://cirrus-ci.com/task/6170839912022016): TheCharlatan: ACK ca2e4ba Tree-SHA512: e3584cae4f50bf2bc6c824bfaddfe683ef6a17d16138d0cbcc544b98bd64d5d7353b0826b1e8cf16e12410e27b0fcedde27100d4241b7cc194cd4465c8175a5b

hebasto · 2024-10-09T09:16:07Z

Combining with #30911 produces even more of a speedup (with Make, ninja is about the same).

Why are ninja builds not affected?

theuni · 2024-10-09T16:20:46Z

Combining with #30911 produces even more of a speedup (with Make, ninja is about the same).

Why are ninja builds not affected?

Ninja builds are sped up significantly by pch (this PR), it's the combo with #30911 doesn't seem to have much effect.

These are all kernel headers which required the most amount of frontend parsing as measured by clang's -ftime-trace option.

ccache supports setting key=value options as of v4.8. Prior to that, setting sloppiness would've required substantial hacking. For 4.8+: - Set the required sloppiness options - Set "-Xclang -fno-pch-timestamp" for clang Both options are documented here: https://ccache.dev/manual/latest.html

PCH is incompabible with iwyu: error: include-what-you-use does not support PCH It seems likely that it'd mess with clang-tidy as well.

laanwj · 2024-10-17T08:47:46Z

Concept ACK. It's nice that CMAKE provides a built-in abstraction for this so there's no need for really ugly stuff.

hebasto

Concept ACK.

Tested and benchmarked on Ubuntu 23.10 with CMake 3.27.4 using the following command:

$ rm -rf build && cmake -B build -G Ninja --preset dev-mode -DWITH_MULTIPROCESS=OFF -DWITH_CCACHE=OFF && time cmake --build build -j 16

The master branch @ 5fb9455:

real	6m46.350s

this PR @ 3cec350:

real	6m10.958s

Do you want to keep the base_precompiled target or use the PRECOMPILE_HEADERS property on per-target basis?

hebasto · 2024-10-22T13:15:33Z

CMakeLists.txt

+else()
+  set(pch_status ON)
+endif()
+message("Precompiled-headers ................... ${pch_status}")


nit:

Suggested change

message("Precompiled-headers ................... ${pch_status}")

message("Precompiled headers ................... ${pch_status}")

edilmedeiros · 2024-10-23T19:41:14Z

Concept ACK
Code LGTM.

rm -rf build && cmake -B build -DBoost_INCLUDE_DIR=/opt/local/libexec/boost/1.78/include -DCMAKE_BUILD_TYPE=Debug -DWITH_CCACHE=OFF -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_UTIL_CHAINSTATE=ON && /usr/bin/time cmake --build build -j 11

Master at ffe4261
1m56,95s real

This PR at 3cec350
1m32,76s real

With ccache 4.10:

rm -rf build && cmake -B build -DBoost_INCLUDE_DIR=/opt/local/libexec/boost/1.78/include -DCMAKE_BUILD_TYPE=Debug -DWITH_CCACHE=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_UTIL_CHAINSTATE=ON && /usr/bin/time -h cmake --build build -j 11

Master at ffe4261
2m18,47s real

This PR at 3cec350
1m52,09s real

All running on a MacBook Pro M3 Pro 18GB with MacOS Sonoma 14.6.1.
cmake version 3.29.5.
Interesting that ccache made my build slower.

laanwj · 2024-10-24T18:03:08Z

Complete build on RPi5 with NVME hat, 8GB memory:

$ cmake -B build  -G Ninja --preset=dev-mode  -DWITH_CCACHE=OFF --toolchain /data/tmp/bitcoin/depends/aarch64-unknown-linux-gnu/toolchain.cmake 
$ time ninja -C build

(PR with pch 3cec350)
real 33m33.235s
user 127m24.712s
sys 4m22.753s

(base commit 5fb9455)
real 37m39.927s
user 144m17.888s
sys 3m26.133s

About 10% faster (in real time), not as impressive as some results on faster machines, but still nice to have.

(did the same bechmark on SD card instead of NVME, the difference is not significant, CPU really dominates instead of I/O here)

Interesting that ccache made my build slower.

well-ccache speeds up repeated builds; it makes sense that if it cannot cache anything it's slightly slower, as it has to store all the input and outputs on disk

fanquake · 2024-11-05T16:31:32Z

src/CMakeLists.txt

@@ -75,6 +75,76 @@ add_subdirectory(secp256k1)
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 string(APPEND CMAKE_C_COMPILE_OBJECT " ${APPEND_CPPFLAGS} ${APPEND_CFLAGS}")

+


Overall, I think this is a reasonable approach because it pretty much just works with no fiddling. The main drawback is that over time the headers list will grow stale, so it'll require some attention now and then.

Not sure if here, but it'd seem useful to document some (minified) version of the PR description in regards to maintaining this going forward. If anything to prevent if from being too arbtrary.

Yes. To be clear, I really don't like the arbitrary nature of this at all. I was hoping someone would have a better idea for an approach to selection.

I really meant this as an RFC because I agree that what's here is a pretty crappy solution. But it's clear that pch is a win, so I think we should come up with something.

DrahtBot · 2024-11-06T11:37:53Z

🐙 This pull request conflicts with the target branch and needs rebase.

BrandonOdiwuor

Concept ACK using pre-compiled headers to speed up compilation

willcl-ark

Took a first pass over this. It seems to work for me locally, both with and without ccache.

I see both clang and gcc running faster with this change than merge-base @ 5fb9455 (with clang being much faster than gcc in both cases):

Summary
  env CCACHE_DISABLE=1 CC=clang CXX=clang++ just build (commit = 3cec3508a79c08db5b97e93bc4225ccdd7d32ad8) ran
    1.32 ± 0.02 times faster than env CCACHE_DISABLE=1 CC=clang CXX=clang++ just build (commit = 5fb94550638)
    1.43 ± 0.00 times faster than env CCACHE_DISABLE=1 CC=gcc CXX=g++ just build (commit = 3cec3508a79c08db5b97e93bc4225ccdd7d32ad8)
    1.53 ± 0.03 times faster than env CCACHE_DISABLE=1 CC=gcc CXX=g++ just build (commit = 5fb94550638)

Command	Mean [s]	Min [s]	Max [s]	Relative
`env CCACHE_DISABLE=1 CC=clang CXX=clang++ just build (commit = 5fb94550638)`	178.672 ± 2.623	176.287	181.481	1.32 ± 0.02
`env CCACHE_DISABLE=1 CC=clang CXX=clang++ just build (commit = 3cec3508a79c08db5b97e93bc4225ccdd7d32ad8)`	134.888 ± 0.242	134.672	135.150	1.00
`env CCACHE_DISABLE=1 CC=gcc CXX=g++ just build (commit = 5fb94550638)`	206.191 ± 4.083	203.754	210.905	1.53 ± 0.03
`env CCACHE_DISABLE=1 CC=gcc CXX=g++ just build (commit = 3cec3508a79c08db5b97e93bc4225ccdd7d32ad8)`	193.261 ± 0.185	193.056	193.416	1.43 ± 0.00

I too can't think of a cleaner way of implementing this, or making it more automated than using a manual list.

Left one comment about adding flags for gcc

willcl-ark · 2024-11-26T16:37:21Z

cmake/ccache.cmake

+      )
+      if(NOT CCACHE_TEST_RESULT)
+        list(APPEND CMAKE_CXX_COMPILER_LAUNCHER sloppiness=pch_defines,time_macros)
+        # According to the ccache docs clang requires this option.


They also give GCC flags. Perhaps here we can therefore use something like:

diff --git a/cmake/ccache.cmake b/cmake/ccache.cmake index 08f078e627b..cb7059884fa 100644 --- a/cmake/ccache.cmake +++ b/cmake/ccache.cmake @@ -35,7 +35,12 @@ if(NOT MSVC) if(NOT CCACHE_TEST_RESULT) list(APPEND CMAKE_CXX_COMPILER_LAUNCHER sloppiness=pch_defines,time_macros) # According to the ccache docs clang requires this option. - try_append_cxx_flags("-Xclang -fno-pch-timestamp" TARGET core_interface SKIP_LINK) + if(CMAKE_CXX_COMPILER_ID STREQUAL "Clang") + try_append_cxx_flags("-Xclang -fno-pch-timestamp" TARGET core_interface SKIP_LINK) + elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + try_append_cxx_flags("-fpch-preprocess" TARGET core_interface SKIP_LINK) + endif() else() list(APPEND configure_warnings "For pre-compiled headers support with ccache, version 4.8+ is required. Disabling pch.") set(CMAKE_DISABLE_PRECOMPILE_HEADERS ON)

DrahtBot · 2025-02-24T00:50:43Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2025-05-25T01:51:14Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

fanquake · 2025-07-02T13:16:44Z

Closing for now. Could re-open if you want to circle back to looking at this.

theuni added the Build system label Oct 7, 2024

DrahtBot added the CI failed label Oct 7, 2024

This was referenced Oct 8, 2024

wip: Split fuzz binary (take 2) #30882

Closed

build: Enhance Ccache performance across worktrees and build trees #30861

Closed

build: Unify -logsourcelocations format #30811

Merged

DrahtBot mentioned this pull request Oct 8, 2024

build: Introduce internal kernel library #28690

Open

theuni mentioned this pull request Oct 8, 2024

refactor: include the proper header rather than forward-declaring RemovalReasonToString #31058

Merged

theuni force-pushed the cmake-pch branch from 78fad02 to d03ee28 Compare October 8, 2024 18:44

DrahtBot removed the CI failed label Oct 8, 2024

theuni added 5 commits October 9, 2024 16:25

build: Add a rough attempt at pre-compiled headers

92e49c7

These are all kernel headers which required the most amount of frontend parsing as measured by clang's -ftime-trace option.

build: display pch status in CMake output

09716ef

guix: disable pre-compiled headers

cbe297f

ci: disable pch for tidy/iwyu builds

3cec350

PCH is incompabible with iwyu: error: include-what-you-use does not support PCH It seems likely that it'd mess with clang-tidy as well.

theuni force-pushed the cmake-pch branch from af80761 to 3cec350 Compare October 9, 2024 16:25

hebasto reviewed Oct 22, 2024

View reviewed changes

bitcoin deleted a comment from syaifulnizamiphone7 Oct 22, 2024

fanquake reviewed Nov 5, 2024

View reviewed changes

DrahtBot added the Needs rebase label Nov 6, 2024

BrandonOdiwuor reviewed Nov 20, 2024

View reviewed changes

willcl-ark reviewed Nov 27, 2024

View reviewed changes

fanquake marked this pull request as draft February 25, 2025 14:54

fanquake closed this Jul 2, 2025

	message("Precompiled-headers ................... ${pch_status}")
	message("Precompiled headers ................... ${pch_status}")

		@@ -75,6 +75,76 @@ add_subdirectory(secp256k1)
		set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
		string(APPEND CMAKE_C_COMPILE_OBJECT " ${APPEND_CPPFLAGS} ${APPEND_CFLAGS}")

RFC: build: support for pre-compiled headers. #31053

RFC: build: support for pre-compiled headers. #31053

Uh oh!

Conversation

theuni commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

theuni commented Oct 7, 2024

Uh oh!

TheCharlatan commented Oct 7, 2024

Uh oh!

DrahtBot commented Oct 7, 2024

Uh oh!

fanquake commented Oct 8, 2024

Uh oh!

theuni commented Oct 8, 2024

Uh oh!

theuni commented Oct 8, 2024

Uh oh!

hebasto commented Oct 9, 2024

Uh oh!

theuni commented Oct 9, 2024

Uh oh!

laanwj commented Oct 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 22, 2024

Choose a reason for hiding this comment

Uh oh!

edilmedeiros commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

theuni Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 6, 2024

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Feb 24, 2025

Uh oh!

DrahtBot commented May 25, 2025

Uh oh!

fanquake commented Jul 2, 2025

Uh oh!

Uh oh!

theuni commented Oct 7, 2024 •

edited

Loading

DrahtBot commented Oct 7, 2024 •

edited

Loading

laanwj commented Oct 17, 2024 •

edited

Loading

hebasto left a comment •

edited

Loading

edilmedeiros commented Oct 23, 2024 •

edited

Loading

laanwj commented Oct 24, 2024 •

edited

Loading