Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MANUAL SQUASH] Precompiled headers support #13355

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Desour
Copy link
Member

@Desour Desour commented Mar 25, 2023

Recent CMake has a target_precompile_headers function that makes using precompiled headers pretty trivial.
(For comparison, see #8643. (cc @numberZero))

Goal: Faster compilation, climate protection, less turbine noise.

1st half of commit:
Moves sha256.c to lib/. This is needed because precompiled headers don't work (assume C) if we're mixing C and C++.

2nd half of commit:
Precompiled header option. (Requires new enough cmake.)

Which headers are precompiled:

  • We only want to precompile headers that seldom or never change. Otherwise we'd have to recompile the precompiled header and recompile everything else when the header changes. And ccache can't know if the precompiled header is different, so even just switching between branches would be horribly painful.
  • => Only stdlib and lib headers are precompiled (excluding irrlicht).
  • I've taken the sdlib headers from Add precompiled header support #8643, and added jsoncpp. (Could add more.)
  • The list of headers is read from a file (src/precompiled_headers.txt). The user can configure this.

To do

This PR is a Ready for Review.

How to test

  • If you're using ccache, add to your ccache.conf: sloppiness = pch_defines,time_macros
  • $ cmake . -DPRECOMPILE_HEADERS=1
  • $ make -j<you choose>

Measure speed:

  • $ cmake . -DCMAKE_CXX_COMPILER=/usr/bin/g++
  • $ make -B -j5
  • Results: (may be inaccurate)
    • No precompile:
      real	7m31,836s
      user	34m38,999s
      sys	1m53,541s
      
    • Precompile:
      real	6m16,476s
      user	28m39,059s
      sys	1m32,657s
      
    • => ca. 17% faster

@Desour Desour added the @ Build CMake, build scripts, official builds, compiler and linker errors label Mar 25, 2023
@lhofhansl
Copy link
Contributor

I'd be worried the additional fragility in the process is not warranted by a 17% compilation time decrease.

@numberZero
Copy link
Contributor

numberZero commented Mar 26, 2023

I tested this (8812b79) with 3 build directories (warm-up, PCH disabled, and PCH enabled) on my system (Ryzen 7 5800X). Here is time comparison:

Target Real, no PCH Real, PCH Real, diff CPU, no PCH CPU, PCH CPU, diff
Clean build 56 48 −15% 785 629 −20%
Rebuild after touching client/content_mapblock.cpp 6.0 5.6 −6% 6.0 5.6 −6%
Rebuild after touching client/tile.h 41 35 −16% 542 437 −19%

Time is in seconds. Time diff is percentage of “no PCH.”

17% faster

Corresponds to −17% “Real, diff” (Clean build) in my format. Which is just a bit better than what I get.

Here are some details:

Without PCH

$ cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DRUN_IN_PLACE=ON -DPRECOMPILE_HEADERS=OFF
<...>
[482/482] Linking CXX executable <...>/bin/minetest
real    0m56,241s
user    12m13,479s
sys     0m51,037s

$ touch src/client/content_mapblock.cpp
<...>
[4/4] Linking CXX executable <...>/bin/minetest
real    0m5,991s
user    0m5,480s
sys     0m0,510s

$ touch src/client/tile.h
<...>
[178/178] Linking CXX executable <...>/bin/minetest
real    0m41,414s
user    8m29,035s
sys     0m32,512s

With PCH

$ cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DRUN_IN_PLACE=ON -DPRECOMPILE_HEADERS=ON
<...>
[483/483] Linking CXX executable <...>/bin/minetest
real    0m48,005s
user    9m41,919s
sys     0m46,652s

$ touch src/client/content_mapblock.cpp
<...>
[6/6] Linking CXX executable <...>/bin/minetest
real    0m5,616s
user    0m5,125s
sys     0m0,508s

$ touch src/client/tile.h
<...>
[177/177] Linking CXX executable <...>/bin/minetest
real    0m34,724s
user    6m47,845s
sys     0m29,482s

Conclusion

While single source file updates are dominated by link time, there is a clear speedup for header updates which tend to require massive recompilation, and for full rebuilds too.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
lib/sha256/CMakeLists.txt Outdated Show resolved Hide resolved
@Desour
Copy link
Member Author

Desour commented Mar 26, 2023

I'd be worried the additional fragility in the process is not warranted by a 17% compilation time decrease.

It's optional and marked as experimental. And CI doesn't use it, so missing includes and similar should usually be found.

Thanks for testing, @numberZero!

@numberZero
Copy link
Contributor

missing includes

Which may very well happen without any PCH because e.g. today, <vector> may include <cstdint> so your code will compile fine... until STL upgrade.

@rubenwardy rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label May 27, 2023
@Zughy
Copy link
Member

Zughy commented Jun 11, 2023

@Desour rebase needed

@Zughy Zughy added the Rebase needed The PR needs to be rebased by its author. label Jun 11, 2023
@Desour Desour removed the Rebase needed The PR needs to be rebased by its author. label Jun 11, 2023
@Zughy Zughy added Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR and removed Concept approved Approved by a core dev: PRs welcomed! labels Jan 22, 2024
Precompiled headers don't work if we're not a pure C++ project.
@Desour Desour changed the title [NOSQUASH] Precompiled headers support [MANUAL SQUASH] Precompiled headers support Feb 7, 2024
with the exception of <filesystem>, because we don't use it yet, and it might be big
@Desour
Copy link
Member Author

Desour commented Feb 7, 2024

Rebased and comments addressed.

@sfan5 sfan5 self-requested a review March 16, 2024 20:32
@sfan5 sfan5 added the Rebase needed The PR needs to be rebased by its author. label May 15, 2024
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me but needs a rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Build CMake, build scripts, official builds, compiler and linker errors One approval ✅ ◻️ Performance Rebase needed The PR needs to be rebased by its author. Supported by core dev Not on the roadmap, yet some core dev decided to take care of this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants