Improve PCH and enable PCH for Catch2 and tests

[vittorioromeo]

tl;dr:

• Compilation speed unchanged with -DSFML_BUILD_TEST_SUITE=1 -DSFML_ENABLE_PCH=0
• Significant speedup with -DSFML_BUILD_TEST_SUITE=1 -DSFML_ENABLE_PCH=1

Full analysis logs available here:

• https://gist.github.com/vittorioromeo/480aa5ca9b940754844b07b4ea69c70e

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.13%. Comparing base (6eaf300) to head (ba39ec3).
Report is 33 commits behind head on master.

❗ Current head ba39ec3 differs from pull request most recent head fe735e9. Consider uploading reports for the commit fe735e9 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2894      +/-   ##
==========================================
+ Coverage   43.65%   44.13%   +0.47%     
==========================================
  Files         253      254       +1     
  Lines       20940    21288     +348     
  Branches     5139     5214      +75     
==========================================
+ Hits         9142     9395     +253     
- Misses      10785    10906     +121     
+ Partials     1013      987      -26     

see 125 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2386653...fe735e9. Read the comment docs.

[eXpl0it3r]

Fails to build for me on Windows with VS2022:

cmake -B build -S . -DSFML_BUILD_TEST_SUITE=1 -DSFML_ENABLE_PCH=1
cmake --build build

MSBuild version 17.8.3+195e7f5a3 for .NET Framework

  1>Checking Build System
  Building Custom Rule SFML/build/_deps/catch2-src/src/CMakeLists.txt
  cmake_pch.cxx
SFML\src\SFML\PCH.hpp(31,10): error C1083: Cannot open include file: 'SFML/Config.hpp': No such file o
r directory [SFML\build\_deps\catch2-build\src\Catch2.vcxproj]
  (compiling source file 'CMakeFiles/Catch2.dir/cmake_pch.cxx')

  Building Custom Rule SFML/src/SFML/System/CMakeLists.txt
  cmake_pch.cxx
  C:\Program Files (x86)\Windows Kits\10\Include\10.0.22000.0\um\dinput.h: DIRECTINPUT_VERSION undefined. Defaulting to ve
  rsion 0x0800
SFML\src\SFML\PCH.hpp(74,10): error C1083: Cannot open include file: 'catch2/catch_test_macros.hpp': N
o such file or directory [SFML\build\src\SFML\System\sfml-system.vcxproj]
  (compiling source file 'CMakeFiles/sfml-system.dir/cmake_pch.cxx')

[vittorioromeo]

@eXpl0it3r: hmm... it's concerning that the CI didn't catch that, to be honest. I wonder why...

I will try to reproduce locally, I guess it's some headers getting copied too late

[eXpl0it3r]

Are we running any build with tests AND PCH enabled in the CI?

[vittorioromeo]

Are we running any build with tests AND PCH enabled in the CI?

Duh, I always forget we are not. We should probably add a job or two...

[vittorioromeo]

@eXpl0it3r: this seems to solve the build on Windows, if you have any suggestions on a more elegant solution I'm all ears :)

[eXpl0it3r]

I wonder if there isn't some include directory variable thingy from the target we can use. Maybe @ChrisThrasher has an idea.

[eXpl0it3r]

Win 11 / VS 2022 (no Ninja or parallelization)

Before this change: 1min 4s
After this change: 37s

(Very scientifically measured with a stop watch 😄)

[ChrisThrasher]

Why not just pass the path to this header to the target_precompiled_headers call within test/CMakeLists.txt? Feels out of place to add non-SFML headers to PCH.hpp. As seen it’s causing downstream complications which I’d rather we avoid.

[vittorioromeo]

You mean to directly pass catch_test_macros instead of PCH.hpp when building Catch?

That was my first approach, but I noticed that Catch2 shares quite a lot of headers in common with SFML (e.g. ostream, algorithm, vector, memory), so it made sense to reuse the existing PCH.hpp rather than creating a new ad-hoc one for Catch2.

The other issue is that we want catch_test_macros to also be part of SFML's own PCH because we use that PCH in our own test suite.

So keeping everything in the same PCH seemed the most reasonable choice here.

We'd still have to keep catch2/catch_test_macros in SFML's own PCH to speed up compilation of our test drivers.

[ChrisThrasher]

Can’t you have multiple PCH sets? And then reuse the PCH set for the core library alongside the PCH for catch_test_macros?

[vittorioromeo]

Can’t you have multiple PCH sets? And then reuse the PCH set for the core library alongside the PCH for catch_test_macros?

I don't see how that solves the issue, it would limit PCH reuse unless I am mistaken...

There are basically three categories of targets:

1. SFML library: needs PCH_STDLIB + PCH_SFML
2. SFML tests: needs PCH_STDLIB + PCH_SFML + PCH_CATCH
3. Catch2 library: needs PCH_STDLIB + PCH_CATCH

Right now, my PCH is PCH_STDLIB + PCH_SFML + PCH_CATCH. This allows me to reuse the same PCH for all three categories of targets.

I definitely want "SFML library" and "SFML tests" to reuse the same PCH, to avoid recompiling two different PCHs -- but that requires that PCH to have access to Catch2 headers anyway.

If we were to go with your solution, then we'd have a few PCH files:

1. PCHStdlib.hpp
2. PCHStdlibSFML.hpp (includes PCHStdlib.hpp)
3. PCHStdlibCatch.hpp (includes PCHStdlib.hpp)
4. PCHStdlibSFMLCatch.hpp (include PCHSFML.hpp and PCHCatch.hpp)

• SFML libraries would use PCHStdlibSFML.hpp.
• SFML tests would use PCHStdlibSFMLCatch.hpp.
• Catch would use PCHStdlibCatch.hpp.

I would be able to use PCHStdlibSFML.hpp for SFML libraries, and use REUSE_FROM from sfml-system.

But if I had to use PCHStdlibSFMLCatch.hpp for tests I would lose the ability to REUSE_FROM from sfml-system, which is a step backwards compared to the current approach. The same also applies to SFML examples.

I don't think going down this route would be better... in the end PCHs are an optimization of build times which already heavily screws up header hygiene, I think it's acceptable to use a few target_include_directories to make everything work when PCHs are enabled.

[coveralls]

Pull Request Test Coverage Report for Build 8954073498

Details

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 51.897%

---

Totals
Change from base Build 8952122764:	0.0%
Covered Lines:	10474
Relevant Lines:	19002

---

💛 - Coveralls

[vittorioromeo]

@ChrisThrasher: any feedback on this? It makes a significant difference on my machine

[ChrisThrasher]

Sorry, but I'm not in favor of this PR. I think the complexity it adds outweighs whatever benefits it may have. A clean build of the test suite (including Catch2) takes under 10 seconds on my 2021 laptop. Adding complexity to improve compile time for users is one thing but I'm much less motivated to add complexity to improve compile times for the small number of people, myself included, who are building the tests on a regular basis.

[vittorioromeo]

Sorry, but I'm not in favor of this PR. I think the complexity it adds outweighs whatever benefits it may have. A clean build of the test suite (including Catch2) takes under 10 seconds on my 2021 laptop. Adding complexity to improve compile time for users is one thing but I'm much less motivated to add complexity to improve compile times for the small number of people, myself included, who are building the tests on a regular basis.

What exactly do you find most complicated about this PR, and what do you think would need to be simplified in order for you to consider approving it?

[ChrisThrasher]

I can't picture a way of transforming this PR into something I'd be willing to approve

[vittorioromeo]

Win 11 / VS 2022 (no Ninja or parallelization)

Before this change: 1min 4s After this change: 37s

(Very scientifically measured with a stop watch 😄)

I think it's quite sad to ignore these sort of improvements. I also think we should start using PCH as our default in CI jobs and only have one or two sanity-check non-PCH jobs to save time and computing power (let's be green!).

Anyone else on my side? @eXpl0it3r @Bromeon @binary1248