8345801: C2: Clean up include statements to speed up compilation when touching type.hpp

[chhagedorn]

I've noticed that after touching type.hpp in Valhalla, it requires more than 7 minutes to build hotspot again on my machine. I would have suspected that we mostly recompile C2/opto source files. But that is far from the truth: A lot of unrelated source files must be recompiled, including, for example, C1, JFR, or runtime files.

In mainline, the impact is not that severe. But it still requires around 1 minute to build hotspot again on my machine after touching type.hpp. I've had a look at the include chains and removed quite a lot of unused includes. For the active includes, the most impact had including output.hpp inside c2compiler.hpp. This set up the following dependency chain:

... more C1 files
  #include "c1/c1_Compilation.hpp"
    #include "compiler/compilerDefinitions.inline.hpp"
      #include "opto/c2compiler.hpp"
        #include "opto/output.hpp"    <------------ Problematic include
          #include "opto/c2_CodeStubs.hpp"
            #include "opto/compile.hpp"
              ... more opto files and eventually type.hpp

This means that a lot of C1 files also need to be re-compiled as well as some more source file that include compiler/compilerDefinitions.inline.hpp. I cut this dependency chain by removing the include of opto/output.hpp in opto/c2compiler.hpp and moved the constant initial_const_capacity from output.hpp to c2Compiler.hpp which seemed to be the only reason why we have the include in place. After this change, I was required to add some missing includes that were included transitively before.

The final mainline patch could also be applied to the current Valhalla repository (with some small tweaks). The results were quite promising. I could bring the compilation time on my machine significantly down in mainline and especially in Valhalla after touching type.hpp:

• Mainline: ~1min -> ~40s (1.5 times faster)
• Valhalla: ~7min -> ~40s (10.5 times faster)

I've only focused on type.hpp here but I guess other includes in the JIT compiler area or other parts of hotspot could also be revisited to possibly speed up the compilation after touching some header files.

Testing:

• Oracle CI
• GHA

Thanks,
Christian

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8345801: C2: Clean up include statements to speed up compilation when touching type.hpp (Enhancement - P4)

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer)
• Dean Long (@dean-long - Reviewer)
• Julian Waters (@TheShermanTanker - Committer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22658/head:pull/22658
$ git checkout pull/22658

Update a local copy of the PR:
$ git checkout pull/22658
$ git pull https://git.openjdk.org/jdk.git pull/22658/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22658

View PR using the GUI difftool:
$ git pr show -t 22658

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22658.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back chagedorn! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@chhagedorn This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8345801: C2: Clean up include statements to speed up compilation when touching type.hpp

Reviewed-by: kvn, dlong, jwaters

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 42 new commits pushed to the master branch:

• a21d21f: 8345609: [C1] LIR Operations with one input should be implemented as LIR_Op1
• 2382a2d: 8345661: Simplify page size alignment in code heap reservation
• 076bfa6: 8345656: Move os alignment functions out of ReservedSpace
• 2826838: 8345658: WB_NMTCommitMemory redundantly records an NMT tag
• c9ec271: 8345800: Update copyright year to 2024 for serviceability in files where it was missed
• 8e0f929: 8345805: Update copyright year to 2024 for other files where it was missed
• f88c1c6: 8345773: Class-File API debug printing capability
• e88e793: 8343148: C2: Refactor uses of "PhaseValue::con() + PhaseIdealLoop::set_ctrl()" into separate method
• 1e9204f: 8345273: Fix -Wzero-as-null-pointer-constant warnings in s390 code
• c40140e: 8334581: Remove no-arg constructor BasicSliderUI()
• ... and 32 more: https://git.openjdk.org/jdk/compare/153dc6d84300e4c3446e33be820c15336cf45e72...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[openjdk]

@chhagedorn The following labels will be automatically applied to this pull request:

• graal
• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (9d7b8e43)

[vnkozlov]

Good.

[dean-long]

It seems weird to have these inlined Compile methods in node.hpp. Could we clean this up separately, maybe by moving them to compile.inline.hpp?

[chhagedorn]

I was confused about that as well. There is currently no compile.inline.hpp but maybe we should introduce one for such cases. I can file an RFE to clean this up separately.

[dean-long]

Looks good, as long as you tested w/ and w/o precompiled headers.

[chhagedorn]

Thanks @vnkozlov and @dean-long for your reviews!

as long as you tested w/ and w/o precompiled headers.

That's a good point. I thought it was covered by our CI but I've run now a separate testing with --disable-precompiled-headers which looked good.

[TheShermanTanker]

It takes just 7 minutes to compile and link HotSpot for you? If only I had that kind of luxury...

[chhagedorn]

Thanks @TheShermanTanker for your review! The measured numbers are only when starting a hotspot build again after touching type.hpp. It takes slightly longer for a full make hotspot in Valhalla.

[chhagedorn]

/integrate

[openjdk]

Going to push as commit 32c8195.
Since your change was applied there have been 99 commits pushed to the master branch:

• 9286018: 8345322: RISC-V: Add concurrent gtests for cmpxchg variants
• 4fc43b0: 8345770: javadoc: API documentation builds are not always reproducible
• ee1c5ad: 8345975: Update SAP SE copyright year to 2024 where it was missed
• 3518b4b: 8344171: Clone and initialize Assertion Predicates in order instead of in reverse-order
• c88e081: 8346160: Fix -Wzero-as-null-pointer-constant warnings from explicit casts
• ab1dbd4: 8346202: Correct typo in SQLPermission
• 6b022bb: 8344453: Test jdk/jfr/event/oldobject/TestSanityDefault.java timed out
• 3b9de11: 8319875: Add macOS implementation for jcmd System.map
• ebb27c2: 8346139: test_memset_with_concurrent_readers.cpp should not use
• c2f0ef5: 8346159: Disable CDS AOTClassLinking tests for JVMCI due to JDK-8345635
• ... and 89 more: https://git.openjdk.org/jdk/compare/153dc6d84300e4c3446e33be820c15336cf45e72...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn Pushed as commit 32c8195.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.