(abandoned) makefile: improve use of precompiled headers

[andrei8l]

Summary

SUMMARY: Build "Improve use of precompiled headers"

Purpose of change

(waste everyone's time with a pipe dream of) Speed-up build from Makefile
Don't pass -DCATCH_CONFIG_ENABLE_PAIR_STRINGMAKER globally

Describe the solution

Main target uses precompiled system headers
Test target uses precompiled catch2 with minor modifications as specified here
Supports both gcc and clang

Describe alternatives you've considered

Making a PR for my meson build definition instead (❤️ meson)

Testing

In progress
Compilation of ncurses targets with gcc and clang worked so far, with both RELEASE=1 and 0

Preliminary benchmark shows 10% improvement on my machine:
make PREFIX=/usr RELEASE=1 PCH=0 ASTYLE=0 LINTJSON=0 -j4 3087.03s user 137.14s system 363% cpu 14:46.61 total
make PREFIX=/usr RELEASE=1 PCH=1 ASTYLE=0 LINTJSON=0 -j4 2779.49s user 122.96s system 376% cpu 12:50.86 total

Additional context

I ported these changes straight from meson and didn't test too much yet with this Makefile
I've also included my hacky script for auto-generating pch.hpp (update-pch.sh)

src/translations.cpp defines the __STRICT_ANSI__ macro, but that's an implementation detail of libstdc++ and has no business in the project's code and also causes an error with precompiled headers. To work around that, the makefile adds the flag -Wno-error=unused-macros

Makefile:414: *** commands commence before first target. Stop.
Makefile:415: *** missing separator (did you mean TAB instead of 8 spaces?). Stop.
I don't know why the mac build make fails to process an undefine instruction. This is not something people should waste time on.
/usr/bin/ld: cannot open output file obj/__bla__.o: No such file or directory clang: error: linker command failed with exit code 1 (use -v to see invocation) () Another great mystery. How does this clang fail to link int main(){}?!

[andrei8l]

CI doesn't seem to like the pre-existing flag -Wno-unknown-warning-option anymore

[kevingranade]

I'm very excited to see what this does to build times!

[kevingranade]

-Werror-no= doesn't seem to be working, if possible we'd prefer to suppress the warning within the source file itself, but if that is untennable for some reason, perhaps -Wno-uused-macros will work.

I believe you need to pass -DCATCH_CONFIG_ALL_PARTS to the compilation process, otherwise it will suppress pch inclusion. i.e. the header that "would have been included" and the header as compiled into the pch must be identical.

This may also break pch in comestibles_test.cpp again since we aren't setting CATCH_CONFIG_ENABLE_PAIR_STRINGMAKER explicitly.

Also see https://ccache.dev/manual/3.7.11.html#_precompiled_headers ccache needs special handling for precompiled headers, which you can see here: https://travis-ci.org/github/CleverRaven/Cataclysm-DDA/jobs/713045770

+ccache --show-stats
cache directory                     /home/travis/.ccache
primary config                      /home/travis/.ccache/ccache.conf
secondary config      (readonly)    /etc/ccache.conf
cache hit (direct)                     0
cache hit (preprocessed)               0
cache miss                             0
called for link                        2
can't use precompiled header         415
files in cache                     10950
cache size                           1.8 GB
max cache size                       2.0 GB

[andrei8l]

-Werror-no= doesn't seem to be working, if possible we'd prefer to suppress the warning within the source file itself, but if that is untennable for some reason, perhaps -Wno-uused-macros will work.

Might depend on compiler version, though I do agree that it's better to disable it with pragma diagnostics

I believe you need to pass -DCATCH_CONFIG_ALL_PARTS to the compilation process, otherwise it will suppress pch inclusion. i.e. the header that "would have been included" and the header as compiled into the pch must be identical.

This isn't necessary when the pch is included explicitly unless the macro changes the API or other odd stuff. Both gcc and clang complain if the pch isn't actually used due to -Winvalid-pch. For example, if you do add CXXFLAGS += -DCATCH_CONFIG_ALL_PARTS, gcc immediately complains:

cc1plus: warning: pch/pch.hpp.gch: not used because `CATCH_CONFIG_ALL_PARTS' is defined [-Winvalid-pch]

This may also break pch in comestibles_test.cpp again since we aren't setting CATCH_CONFIG_ENABLE_PAIR_STRINGMAKER explicitly.

As far as I can tell, that macro only enables extra symbols and is covered by CATCH_CONFIG_ALL_PARTS. In worst case though, the pch isn't used for that one translation unit (and the compiler would complain about it, or error out due to API differences)

Also see https://ccache.dev/manual/3.7.11.html#_precompiled_headers ccache needs special handling for precompiled headers, which you can see here: https://travis-ci.org/github/CleverRaven/Cataclysm-DDA/jobs/713045770

+ccache --show-stats
cache directory                     /home/travis/.ccache
primary config                      /home/travis/.ccache/ccache.conf
secondary config      (readonly)    /etc/ccache.conf
cache hit (direct)                     0
cache hit (preprocessed)               0
cache miss                             0
called for link                        2
can't use precompiled header         415
files in cache                     10950
cache size                           1.8 GB
max cache size                       2.0 GB

I think I'm already covering that entire second bullet point in the link. Is the first one something I can do on my end? I'll look into it

EDIT: looks like I already had that ccache config locally and I'm not seeing any hits for can't use precompiled header during a CCACHE=1 CLANG=1 build

Also I just finished a benchmark on a different system with similar results as in the OP

make PREFIX=/usr RELEASE=1 PCH=1 ASTYLE=0 LINTJSON=0 -j4  2819.51s user 129.98s system 347% cpu 14:08.41 total
make PREFIX=/usr RELEASE=1 PCH=0 ASTYLE=0 LINTJSON=0 -j4  3159.80s user 148.50s system 319% cpu 17:14.87 total

[andrei8l]

By the way, that __STRICT_ANSI__ thing hasn't been updated in 3 years. Might be worth checking if it's still needed with recent versions of mingw-w64 @Qrox

[andrei8l]

ccache does indeed work on my end with PCH=1. I reset the stats, then ran make PCH=1 CCACHE=1 RELEASE=1 ASTYLE=0 LINTJSON=0 -j4, then make clean, and finally the first make again:

❯ ccache -s
cache directory                     /home/andrei/.ccache
primary config                      /home/andrei/.ccache/ccache.conf
secondary config      (readonly)    /etc/ccache.conf
stats updated                       Thu Jul 30 03:13:16 2020
stats zeroed                        Thu Jul 30 02:42:34 2020
cache hit (direct)                   406
cache hit (preprocessed)               8
cache miss                           489
cache hit rate                     45.85 %
called for link                        5
compile failed                         5
cleanups performed                     3
files in cache                     13588
cache size                           3.8 GB
max cache size                       4.2 GB

no pch misses (the failures are from that friendly __STRICT_ANSI__, which I think I managed to fix)

[kevingranade]

So is the next step to wrap all the system header inclusion blocks in #ifndef PCH? The ones in .cpp files anyway i.e.

#ifndef PCH
<algorithm>
<list>
<unordered_map>
#endif

Note if it is I don't want it in this PR, one step at a time.

[andrei8l]

Nope, this is fully set up as it is and there are no further changes needed (at least for native compilation with gcc and clang). The -include/-include-pch instruction ensures that the pch is always the first include in every translation unit, and the include guards in standard headers prevent any issues with double inclusion or whatnot.

[kevingranade]

There's no double inclusion, but we still pay the overhead of inlining all those headers, right? (we just skip compiling the contents)
I might be misunderstanding something about the process, but it seems like the compiler will still need to at least read in the header file to get at the include guards and scan to the terminating #endif if it is included, but if we wrap it in our own #if it should be able to avoid referencing the headers entirely except at precompile time.

I'll do some tests and see if there's anything measurable once I have a little while to look at it.

[andrei8l]

Ah ok, I get your point. The issue with that is you'll have to rebuild the pch every time you add a new (project-wide) include which necessarily triggers a full rebuild and might counteract any time savings. The alternative would be to teach people to add new includes outside of the #if PCH...#endif block and that probably has its own pitfalls.

[kevingranade]

every time you add a new (project-wide) include

Is this something that's going to happen more than once every few months? I think we're pretty nearly at saturation already.

which necessarily triggers a full rebuild

We currently trigger nearly-full rebuilds pretty constantly anyway, CI builds in particular get hit really hard with this, but this way those full rebuilds are much faster.

As for the ccache thing, I think you want
--set-config=sloppiness=pch_defines,time_macros added to each compile line, either that or put the equivalent in a ccache config file somewhere and tell ccache where to find it.

[Qrox]

By the way, that __STRICT_ANSI__ thing hasn't been updated in 3 years. Might be worth checking if it's still needed with recent versions of mingw-w64

I doubt it's doing anything in the code now since <cstdlib> is already included before __STRICT_ANSI__ is defined.

Edit: removed it and it still compiles on MinGW-w64.

[andrei8l]

Thanks @Qrox. I'll leave it as it is in this PR since the #ifdef hack seems to have satisfied all CI checks.

[andrei8l]

So travis' old clang doesn't understand -fno-pch-timestamp while new clang versions don't like ccache'd precompiled headers.

What do?

[andrei8l]

Alright, I'm done. I've pushed another hack for clang+ccache that seems to work locally.

Hacking Makefiles is extremely unfun and I'd rather be doing something else, so if this doesn't work for CI, I'm going to close the PR

[kevingranade]

If you give up please leave it open for a bit and someone is going to take it up. Regardless thanks a ton for grinding away on this, it's going to have a really high impact.

[andrei8l]

Sorry, I couldn't get it to work on travis (works for me™). Maybe smarter people can figure it out

[kevingranade]

Well you got everything but the XCode build, I'm sure we can sort that out, thanks a ton.

[andrei8l]

According to this
BSD-variants of make treat spaces as tabs (i.e. they consider them to define rules instead of script)

Allowing <space> characters as well as <tab> characters to delimit command lines (BSD)

So maybe removing the indentations from the entire ifeq ($(PCH), 1) block would fix the mac build

[anothersimulacrum]

It also works now, with the undefines removed.

[andrei8l]

It also works now, with the undefines removed.

Nice! There's still this bizarre error that prevents clang build from using PCH:

Makefile:418: your clang version does not support -fno-pch-timestamp: ld: can't open output file for writing: obj/__bla__.o, errno=2 for architecture x86_64 clang: error: linker command failed with exit code 1 (use -v to see invocation) ()

Adding -c to the CLANGVER command (to skip the linker) might fix it

[kevingranade]

One of us can dive into resolving the remaining issues in another PR, I just want this one in the can!

[andrei8l]

I hope it turns out to be useful. Thanks for merging it

[ZhilkinSerg]

Actually I think it would've been succeed on Travis if not for a time limit. We'll see how it will work on Jenkins.