8257731: Remove excessive include of stubRoutines.hpp

[iklam]

stubRoutines.hpp is unnecessarily included by thread.hpp and copy.hpp. This causes a large number of header files related to native code generation to be included by almost all HotSpot .o files.

• Before the fix, stubRoutines.hpp is included by 803 .o files.
• After the fix, stubRoutines.hpp is included by 117 .o files.

The total number of included header files decreased from 252893 to 247631, or about 2%.

Note: the main change is the removal of stubRoutines.hpp in thread.hpp and copy.hpp. Unfortunately I have to fix quite a few missing dependencies in other source files that are revealed by the removal.

Testing: I've tested tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5 on mach5, plus locally building arm, aarch64, s390, ppc64, and zero.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8257731: Remove excessive include of stubRoutines.hpp

Reviewers

• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 9baf9a5
• Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 9baf9a5

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1610/head:pull/1610
$ git checkout pull/1610

[bridgekeeper]

👋 Welcome back iklam! 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]

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

• hotspot
• serviceability

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.

[iklam]

/label remove serviceability

[openjdk]

@iklam
The serviceability label was successfully removed.

[mlbridge]

Webrevs

• 01: Full - Incremental (c4f2d8e)
• 00: Full (9baf9a5)

[coleenp]

Looks good to me, if you did a test build of zero and maybe ppc at least.

[openjdk]

@iklam 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:

8257731: Remove excessive include of stubRoutines.hpp

Reviewed-by: coleenp, kvn

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

[vnkozlov]

Make sure all builds are passed.

[iklam]

Make sure all builds are passed.

@vnkozlov @coleenp I've tested tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5 on mach5, plus locally building arm, aarch64, s390, ppc64, and zero.

[vnkozlov]

Make sure all builds are passed.

@vnkozlov @coleenp I've tested tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5 on mach5, plus locally building arm, aarch64, s390, ppc64, and zero.

Good. Please, list testing in your PR descriptions so that we know what you did.

[iklam]

Make sure all builds are passed.

@vnkozlov @coleenp I've tested tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5 on mach5, plus locally building arm, aarch64, s390, ppc64, and zero.

Good. Please, list testing in your PR descriptions so that we know what you did.

Done. I updated the PR descriptions.

[shipilev]

I have a suggestion: let's wait until JDK 16 forks for stabilization before we do any #include reshuffling? There are build breakages from these changes, so it would be nice to have these breakages away from the stabilizing repository.

[iklam]

I have a suggestion: let's wait until JDK 16 forks for stabilization before we do any #include reshuffling? There are build breakages from these changes, so it would be nice to have these breakages away from the stabilizing repository.

OK, I'll push in JDK 17.

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

On 4/12/2020 7:46 pm, Aleksey Shipilev wrote:

On Fri, 4 Dec 2020 00:48:16 GMT, Ioi Lam <iklam at openjdk.org> wrote:

Make sure all builds are passed.

@vnkozlov @coleenp I've tested tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5 on mach5, plus locally building arm, aarch64, s390, ppc64, and zero.

Good. Please, list testing in your PR descriptions so that we know what you did.

Make sure all builds are passed.

@vnkozlov @coleenp I've tested tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5 on mach5, plus locally building arm, aarch64, s390, ppc64, and zero.

Good. Please, list testing in your PR descriptions so that we know what you did.

Done. I updated the PR descriptions.

I have a suggestion: let's wait until JDK 16 forks for stabilization before we do any `#include` reshuffling? There are build breakages from these changes, so it would be nice to have these breakages away from the stabilizing repository.

With sufficient pre-integration testing this should not be an issue. It
seems for JDK-8257563 no GitHub actions were run and so numerous build
variants were missed.

David

[cl4es]

With sufficient pre-integration testing this should not be an issue. It
seems for JDK-8257563 no GitHub actions were run and so numerous build
variants were missed.

And since there's likely going to be some churn happening in tandem in both mainline and stabilization fork in the coming weeks, wouldn't it be better to get changes like these pushed before rather than right after the JDK 16 stabilization fork is created? The second best option might be to hold off with large cleanups/reshuffling until RDP2 starts.

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

On 5/12/2020 9:32 pm, Claes Redestad wrote:

On Fri, 4 Dec 2020 17:00:57 GMT, Ioi Lam <iklam at openjdk.org> wrote:

I have a suggestion: let's wait until JDK 16 forks for stabilization before we do any `#include` reshuffling? There are build breakages from these changes, so it would be nice to have these breakages away from the stabilizing repository.

I have a suggestion: let's wait until JDK 16 forks for stabilization before we do any `#include` reshuffling? There are build breakages from these changes, so it would be nice to have these breakages away from the stabilizing repository.

OK, I'll push in JDK 17.

With sufficient pre-integration testing this should not be an issue. It
seems for JDK-8257563 no GitHub actions were run and so numerous build
variants were missed.

And since there's likely going to be some churn happening in tandem in both mainline and stabilization fork in the coming weeks, wouldn't it be better to get changes like these pushed _before_ rather than right after the JDK 16 stabilization fork is created? The second best option might be to hold off with large cleanups/reshuffling until RDP2 starts.

I think the point was to defer to this to be 17 only so no churn at all
in 16 RDP1 or RDP2.

Cheers,
David

[mlbridge]

Mailing list message from Claes Redestad on hotspot-dev:

On 2020-12-06 11:15, David Holmes wrote:

And since there's likely going to be some churn happening in tandem in
both mainline and stabilization fork in the coming weeks, wouldn't it
be better to get changes like these pushed _before_ rather than right
after the JDK 16 stabilization fork is created? The second best option
might be to hold off with large cleanups/reshuffling until RDP2 starts.

I think the point was to defer to this to be 17 only so no churn at all
in 16 RDP1 or RDP2.

Seems we're talking past each other:

- The churn I'm referring to is the regular weeks (or months) of bug
fixing and forward-porting that happen after a stabilization fork - not
some highly unlikely churnpocalypse resulting from this patch in particular.

- Getting this into 17 right after the stabilization fork means we have
an increased risk of merge conflicts etc, which means the automatic
forward porting of pushes to 16 will be in peril.

So what I'm suggesting here is to either:

- Get this tested out on all platforms and pushed _now_ to reduce risk
of conflicts during 16 stabilization

OR

- Wait until some time after 16 forks to get past the 16 stabilization
churn to minimize risk of conflicts during stabilization that might
prohibit automatic forward porting.

I have a preference for the former.

/Claes

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

On 6/12/2020 9:23 pm, Claes Redestad wrote:

On 2020-12-06 11:15, David Holmes wrote:

And since there's likely going to be some churn happening in tandem
in both mainline and stabilization fork in the coming weeks, wouldn't
it be better to get changes like these pushed _before_ rather than
right after the JDK 16 stabilization fork is created? The second best
option might be to hold off with large cleanups/reshuffling until
RDP2 starts.

I think the point was to defer to this to be 17 only so no churn at
all in 16 RDP1 or RDP2.

Seems we're talking past each other:

- The churn I'm referring to is the regular weeks (or months) of bug
fixing and forward-porting that happen after a stabilization fork - not
some highly unlikely churnpocalypse resulting from this patch in
particular.

I've no idea what you mean by this.

- Getting this into 17 right after the stabilization fork means we have
an increased risk of merge conflicts etc, which means the automatic
forward porting of pushes to 16 will be in peril.

True this could impact the form of changes in 16 versus 17.

So what I'm suggesting here is to either:

- Get this tested out on all platforms and pushed _now_ to reduce risk
of conflicts during 16 stabilization

OR

- Wait until some time after 16 forks to get past the 16 stabilization
churn to minimize risk of conflicts during stabilization that might
prohibit automatic forward porting.

I have a preference for the former.

Okay. I also prefer the former. The previous issue with this kind of
patch was lack of testing, so as long as we have adequate testing this
kind of refactoring should be a non-issue.

Blocking this kind of change effectively introduces a de-facto RDP0.

David
-----

/Claes

[shipilev]

I don't mind pushing it for JDK 16. I was just suggesting that we defer somewhat intrusive cleanups to JDK 17, as it customary to do (at least in my vicinity) in order to have everyone else build/test based on clean master in the days coming to JDK 16 cutoff. But we could instead pay more attention to the state of master and fix the follow-up build failure bugs even more promptly this week.

[mlbridge]

Mailing list message from Ioi Lam on hotspot-dev:

On 12/6/20 9:44 AM, Aleksey Shipilev wrote:

On Sat, 5 Dec 2020 11:29:41 GMT, Claes Redestad <redestad at openjdk.org> wrote:

I have a suggestion: let's wait until JDK 16 forks for stabilization before we do any `#include` reshuffling? There are build breakages from these changes, so it would be nice to have these breakages away from the stabilizing repository.
OK, I'll push in JDK 17.
With sufficient pre-integration testing this should not be an issue. It
seems for JDK-8257563 no GitHub actions were run and so numerous build
variants were missed.
And since there's likely going to be some churn happening in tandem in both mainline and stabilization fork in the coming weeks, wouldn't it be better to get changes like these pushed _before_ rather than right after the JDK 16 stabilization fork is created? The second best option might be to hold off with large cleanups/reshuffling until RDP2 starts.

Just to be clear -- this is not a "disruptive" commit. It's unlikely to
cause problems when backporting unrelated commits from the mainline back
to JDK 16.

This commit discovers existing problems in the HotSpot code -- many
source files do not explicitly include header files for the APIs they
reference. Instead, they rely on another header file (sometimes many
recursions away) to include the required header files. For example:

c1_LIRAssembler_aarch64.cpp calls StubRoutines::generic_arraycopy().
However, it didn't explicitly include stubRoutines.hpp, but relied on
thread.hpp to do that. After stubRoutines.hpp is removed from
thread.hpp, c1_LIRAssembler_aarch64.cpp fails to compile.

So the majority of the the changes in this commit is *adding* #includes
in the CPP files. This by definition can only cause FEWER build failures
if this commit is pushed into JDK 16.

Conversely, if this commit is not pushed in JDK 16, I can imagine in
some edge scenarios that? an unrelated backport into 16 cannot be
cleanly applied. But this will require a second backport that *removes*
headers from an HPP file (which will be unlikely to be backported in the
first place). In any case, even if this happens, any breakage can be
easily fixed by adding #includes in the backport, and we have such
backports from time to time anyway.

So in my opinion, it doesn't really matter if this commit is in JDK 16
or not.

I don't mind pushing it for JDK 16. I was just suggesting that we defer somewhat intrusive cleanups to JDK 17, as it customary to do (at least in my vicinity) in order to have everyone else build/test based on clean master in the days coming to JDK 16 cutoff. But we could instead pay more attention to the state of master and fix the follow-up build failure bugs even more promptly this week.

I agree with this. In the event this commit breaks something in a build
that I haven't tested, a follow-up commit needs to be pushed. It would
be nice to push this commit after the cutoff to avoid the need to
backport the follow-up commit.

Thanks
- Ioi

[mlbridge]

Mailing list message from Ioi Lam on hotspot-dev:

On 12/6/20 3:23 AM, Claes Redestad wrote:

On 2020-12-06 11:15, David Holmes wrote:

And since there's likely going to be some churn happening in tandem
in both mainline and stabilization fork in the coming weeks,
wouldn't it be better to get changes like these pushed _before_
rather than right after the JDK 16 stabilization fork is created?
The second best option might be to hold off with large
cleanups/reshuffling until RDP2 starts.

I think the point was to defer to this to be 17 only so no churn at
all in 16 RDP1 or RDP2.

Seems we're talking past each other:

- The churn I'm referring to is the regular weeks (or months) of bug
fixing and forward-porting that happen after a stabilization fork - not
some highly unlikely churnpocalypse resulting from this patch in
particular.

- Getting this into 17 right after the stabilization fork means we have
an increased risk of merge conflicts etc, which means the automatic
forward porting of pushes to 16 will be in peril.

So what I'm suggesting here is to either:

- Get this tested out on all platforms and pushed _now_ to reduce risk
of conflicts during 16 stabilization

OR

- Wait until some time after 16 forks to get past the 16 stabilization
churn to minimize risk of conflicts during stabilization that might
prohibit automatic forward porting.

Notwithstanding my claim that this commit is not a disruptive one ....

Do we have a policy of withholding disruptive changes in the mainline
during the RDP timeline?

Thanks
- Ioi

I have a preference for the former.

/Claes

[iklam]

Tested with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5 on mach5, plus locally building arm, aarch64, s390, ppc64, and zero.
/integrate

[openjdk]

@iklam Pushed as commit d4282b0.

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