Consider reworking how PCHs are used #324
Assignees
Labels
codebase quality
Issues that are not bugs, but still might be worth improving (eg, code hygiene or maintainability)
Comments
|
This is your friendly Microsoft Issue Bot. I've seen this issue come in and have gone to tell a human about it. |
grochocki
added
the
codebase quality
|
I have a PR out to address this issue. We'll have to take the risk of regressing CMake/Clang builds for now until we get CI support. |
danbelcher-MSFT
pushed a commit
that referenced
this issue
Apr 18, 2019
Fixes #324 . Description of the changes: In an effort to support other compilers (#109), this change reworks how precompiled headers are handled. For toolchains where precompiled headers are not used, there is unnecessary compilation cost because each source file explicity includes the pch, meaning all system headers in the pch were recompiled for each translation unit. This change modifies the project's files so that each translation unit includes a minimal set of dependent headers. For MSVC users, the precompiled headers option is still enabled and the precompiled header is added to each translation unit using the compiler's Forced Includes option. The end result is that MSVC users still see the same build times, but other toolchains are free to use or not use precompiled headers. Risks introduced Given that our CI build uses MSVC, this change introduces the risk that a system header is added to the pch and the CalcManager project builds correctly, but builds could be broken for other toolsets that don't use pch. We know we want to add support for Clang in our CI build (#211). It seems reasonable to also compile without precompiled headers there so that we can regression test this setup. How changes were validated: Rebuild CalcManager project. Compile time: ~4.5s. Disable precompiled headers, keeping explicit include for pch in each source file. Compile time: ~13s. Remove explicit pch inclusion and add the appropriate headers to each translation unit to allow the project to compile. Compile time: ~8s. Re-enable pch and include it using the Forced Includes compiler option. MSVC compile time: ~4.5s. Minor changes Delete 'targetver.h'. I found this while looking around for system headers in the project. It's unused and unreferenced so let's remove it.
EriWong
pushed a commit
to EriWong/calculator
that referenced
this issue
Jun 5, 2019
…osoft#436) Fixes microsoft#324 . Description of the changes: In an effort to support other compilers (microsoft#109), this change reworks how precompiled headers are handled. For toolchains where precompiled headers are not used, there is unnecessary compilation cost because each source file explicity includes the pch, meaning all system headers in the pch were recompiled for each translation unit. This change modifies the project's files so that each translation unit includes a minimal set of dependent headers. For MSVC users, the precompiled headers option is still enabled and the precompiled header is added to each translation unit using the compiler's Forced Includes option. The end result is that MSVC users still see the same build times, but other toolchains are free to use or not use precompiled headers. Risks introduced Given that our CI build uses MSVC, this change introduces the risk that a system header is added to the pch and the CalcManager project builds correctly, but builds could be broken for other toolsets that don't use pch. We know we want to add support for Clang in our CI build (microsoft#211). It seems reasonable to also compile without precompiled headers there so that we can regression test this setup. How changes were validated: Rebuild CalcManager project. Compile time: ~4.5s. Disable precompiled headers, keeping explicit include for pch in each source file. Compile time: ~13s. Remove explicit pch inclusion and add the appropriate headers to each translation unit to allow the project to compile. Compile time: ~8s. Re-enable pch and include it using the Forced Includes compiler option. MSVC compile time: ~4.5s. Minor changes Delete 'targetver.h'. I found this while looking around for system headers in the project. It's unused and unreferenced so let's remove it.
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
As discussed in #208 (comment):
PCHs in current form are problematic to use due to varying support of build systems. While the most common setup I imagine would be with the MSVS project files included in the repository, #211 adds CMake support and when preparing #212 I manually invoked the compiler, which causes inclusion of many unnecessary header files, which leads to prolonged compilation times.
I can suggest a simple solution to that issue: make the leaf
.cppfiles#includethe minimal set of what they truly need, leavepch.{h,cpp}files intact, then upon building:.cppfiles, inject the header via compiler, e.g.This way you can have regular PCHs without forcing half of STL into each translation unit even when they don't need it.
I'm sure there are other ways of solving that issue as well.
The text was updated successfully, but these errors were encountered: