On Mon, 20 Jan 2025 10:06:16 GMT, Stefan Karlsson <stef...@openjdk.org> wrote:
>> HotSpot uses precompiled headers to speed up non-incremental builds. The >> current mechanism requires all .cpp files to include precompiled.hpp as the >> first non-comment line. This requires HotSpot devs to think about >> precompiled headers when they create new files and when they need to >> update/manage the include sections. >> >> All the compilers that we use support using precompiled headers without >> explicitly including the associated header in the source code. I propose >> that we use that capability instead. >> >> The make files changes to support this are: >> 1) Remove share/precompiled/ from the list of include directories, >> 2) Compiler-specific changes to support this: >> >> *Windows cl*: >> * Add the share/precompiled/ include directory when generating the PCH file. >> * Use /FI to indicate that we should include precompiled.hpp. /Yu says that >> it should use a PCH for precompiled.hpp. /Fp tells the compiler that it >> should pick up the named precompiled header. >> >> From experiments it seems like it doesn't matter what name I pass in to /FI >> and /Yu, they just have to be the same and the specified file doesn't even >> have to exist, as long as we also pass in the /Fp flag we get the benefits >> of the precompiled header. >> >> *gcc*: >> * Use -include to include the precompiled.hpp file. Note that the >> compilation command line will not have share/precompiled in the include path >> and that the precompiled header will still be picked up correctly. >> >> *clang*: >> * No changes needed, the -include-pch line is all that we need. >> >> I've verified that these changes give the expected compilation time >> reductions both by comparing compilation times in our CI but also by running >> individual compilations of .cpp files on Linux, Mac, and Windows. >> >> Note that the compiler will complain if someone now tries to include >> precompiled.hpp explicitly. >> >> Note also that we have a section about precompiled.hpp in the HotSpot style >> guide. If this proposal is accepted I would like to update the style guide >> as a separate RFE. >> >> Tested by letting GHA build and running tier1-2 in our CI pipeline. > > Stefan Karlsson has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - Copyright > - Merge remote-tracking branch 'upstream/master' into > 8347909_remove_precompiled_from_src > - Update make/common/native/CompileFile.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> > - Update make/common/native/CompileFile.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> > - Update make/common/native/CompileFile.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> > - Update make/common/native/CompileFile.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> > - Remove precompiled.hpp include from source files Looks great, thanks for doing this!!! src/hotspot/share/classfile/dictionary.cpp line 1: > 1: Added new-line at start of file, please remove. src/hotspot/share/utilities/events.cpp line 1: > 1: Added new-line at start of file, please remove. ------------- PR Review: https://git.openjdk.org/jdk/pull/23146#pullrequestreview-2562911221 PR Review Comment: https://git.openjdk.org/jdk/pull/23146#discussion_r1922707284 PR Review Comment: https://git.openjdk.org/jdk/pull/23146#discussion_r1922710809
