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

Support pre-compiled headers and interprocedural/link-time optimisation #2978

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

Conversation

Semphriss
Copy link
Member

This adds CMake code to enable pre-compiled headers (PCH) and interprocedural optimisation (IPO, or LTO for link-time optimisation).

PCH

PCH makes builds 50% to 66% faster. A simple time make -j$(nproc) on my 12-core CPU shows:

With PCH:
real    0m58.048s
user    5m33.425s
sys    1m56.666s

Without PCH:
real    1m50.533s
user    15m20.049s
sys    2m56.815s

real is the total time spent, from start to finish, as perceived by the developer. user is the sum of the time spent by each thread, which gives a better idea of what would happen on a single-threaded system. Details

IPO

IPO makes Release builds 15% smaller. I have not tested the runtime performance.


Notes

  • As instructed, I've opened a single PR for both of these.
  • For PCH to work properly, I had to add some missing include guards and include headers in some files.
  • cmake_policy(SET CMP0069 NEW) requires CMake >=3.9, but I did not update cmake_minimum_required to discuss whether or not to keep the policy.

Semphris added 6 commits June 16, 2024 16:43
This speeds up builds by 50% to 66% on my machine.
Release builds are about 15% smaller. Runtime performance untested.
- `src/ports/emscripten.hpp` lacked an include guard.
- MSVC probably defines `STRICT` as a macro.
- Moved Emscripten implementation from header to .cpp file
- See if #undef MSVC-specific macro fixes MSVC builds
- Filter out OpenGL headers on non-HAVE_OPENGL builds
- The MSVC bug from the parent commit wasn't a macro, apparently
- Forgot to commit src/port/emscripten.cpp
- Emscripten doesn't support LTO very well, disable it for now
@Semphriss
Copy link
Member Author

I had to change a few more things in the code for the builds to pass. I changed it to the best of my abilities, but some involve design choices, so it might be good to review them. The diff will show all the changes, but I bumped the minimum CMake version from 3.1 to 3.9 to support policy CMP0069 (needs 3.6) and list(FILTER ...) (requires 3.9). Let me know if you'd like to change something.

@tobbi
Copy link
Member

tobbi commented Jun 19, 2024

I tested this a couple days ago. Seems to be working fine.

CMakeLists.txt Outdated Show resolved Hide resolved
@HybridDog
Copy link
Contributor

This sounds like enabling pre-compiled headers has only advantages and no disadvantage.
If a developer edits a single header file and recompiles the program, is the recompilation faster or slower with pre-compiled headers enabled?

@Semphriss
Copy link
Member Author

Semphriss commented Jun 27, 2024

@HybridDog It's something I've been thinking about, whether or not to enable them by default. They're not really relevant during development, only when making releases, which is IMO the preferred place for lesser-known non-default options to live.

I haven't tried actively developing with those options enabled, but I would presume, especially with LTO, that it substantially slows down development compiling. I'm not sure how large the impact is, and active developers would be better placed to report their experience with that.

@HybridDog
Copy link
Contributor

If they are only relevant for release builds, maybe they can be enabled if and only if the build type is Release or RelWithDebInfo (-DRELEASE).

@Semphriss
Copy link
Member Author

PCH and IPO are now only enabled on Release|RelWithDebInfo builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants