8253270: Limit fastdebug inlining in G1 evacuation #220

kimbarrett · 2020-09-17T08:26:06Z

Please review this change to G1 evacuation to prevent inlining of some functions in debug (particularly, fastdebug) builds. This reduces the size of the generated code to stay away from limits encountered with gcc when compiling for aarch64. As a side benefit, this adds more call frames that might be helpful with debugging.

Testing: tier1
Verified expected inlining on linux-x64 and linux-aarch64 in both release and fastdebug builds.

Progress

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

Issue

JDK-8253270: Limit fastdebug inlining in G1 evacuation

Reviewers

Thomas Schatzl (@tschatzl - Reviewer) ⚠️ Review applies to 25f8ded
Stefan Johansson (@kstefanj - Reviewer)
Albert Mingkun Yang (@albertnetymk - Author)

Download

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

bridgekeeper · 2020-09-17T08:27:32Z

👋 Welcome back kbarrett! 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 · 2020-09-17T08:28:42Z

@kimbarrett The following label will be automatically applied to this pull request: hotspot-gc.

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

mlbridge · 2020-09-17T08:42:29Z

Webrevs

01: Full - Incremental (0039c4a)
00: Full (25f8ded)

tschatzl

Lgtm.

openjdk · 2020-09-17T08:56:20Z

@kimbarrett This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8253270: Limit fastdebug inlining in G1 evacuation

Reviewed-by: tschatzl, sjohanss, ayang

If you would like to add a summary, use the /summary command.
To credit additional contributors, use the /contributor command.
To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there have been 28 commits pushed to the master branch:

8904420: 8253153: Mentioning of "hour-of-minute" in java.time.temporal.TemporalField JavaDoc
52c28b8: 8253239: Disable VS warning C4307
05572e8: 8253350: Remove unimplemented SharedRuntime::clean_*_entry
43019a0: 8253345: Remove unimplemented Arguments::lookup_logging_aliases
6e9efff: 8253344: Remove unimplemented Arguments::check_gc_consistency
83b2411: 8253342: Fix typos in String.transform
86a1640: 8244505: G1 pause time ratio calculation does not consider Remark/Cleanup pauses
11c4ea9: 8253278: Refactor/cleanup oopDesc::*_klass_addr
73c9088: 8249451: Unconditional exceptions clearing logic in compiler code should honor Async Exceptions.
3ef2efb: 8252721: Nested classes in Swing APIs rely on default constructors
... and 18 more: https://git.openjdk.java.net/jdk/compare/9a7dcdcdbad34e061a8988287fe691abfd4df305...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge master into your branch, and then specify the current head hash when integrating, like this: /integrate 89044200cd1eb8ab7747c8861320a291772b3dd3.

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

albertnetymk · 2020-09-17T08:58:37Z

If this workaround is needed due to a compiler bug, I think it's best to include a reference to the corresponding compiler ticket(s) so that it's possible to undo this workaround when the bug is fixed. On second thought, I wonder if it's feasible to just raise the required compiler version, assuming this bug is fixed in later versions of gcc.

mlbridge · 2020-09-17T09:17:15Z

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Sep 17, 2020, at 5:01 AM, Albert Mingkun Yang <ayang at openjdk.java.net> wrote:

On Thu, 17 Sep 2020 08:55:08 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

Please review this change to G1 evacuation to prevent inlining of some functions in debug (particularly, fastdebug)
builds. This reduces the size of the generated code to stay away from limits encountered with gcc when compiling for
aarch64. As a side benefit, this adds more call frames that might be helpful with debugging. Testing: tier1
Verified expected inlining on linux-x64 and linux-aarch64 in both release and fastdebug builds.

Lgtm.

If this workaround is needed due to a compiler bug, I think it's best to include a reference to the corresponding
compiler ticket(s) so that it's possible to undo this workaround when the bug is fixed.

I looked for an existing bug report in gcc's bug tracker, but didn't find
one. It might have been reported in some other aarch64-focused tracker. I
didn't spend too much time looking.

I think the change is reasonable to leave in place permanently. It does
have the mentioned side benefit. It also speeds up compilation of that file
in fastdebug builds by a noticeable amount. (The fastdebug output file
is *huge*.)

On second thought, I wonder if
it's feasible to just raise the required compiler version, assuming this bug is fixed in later versions of gcc.

The JDK-8253169 failure occurs with gcc9.2, but not with gcc10.2, so may
have been fixed already. Or maybe some other code generation change caused
the problem limit to not be reached with the later compiler. I'm in favor
of reporting bugs upstream when they are found, but this one doesn't seem
likely to be worth the effort.

Increasing the mininum required gcc version for linux-aarch64 to 10.x
doesn't seem reasonable at this time, without a much better reason than this.

kstefanj

Looks good.

mlbridge · 2020-09-17T09:18:04Z

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Sep 17, 2020, at 4:58 AM, Thomas Schatzl <tschatzl at openjdk.java.net> wrote:

On Thu, 17 Sep 2020 08:26:06 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

Please review this change to G1 evacuation to prevent inlining of some functions in debug (particularly, fastdebug)
builds. This reduces the size of the generated code to stay away from limits encountered with gcc when compiling for
aarch64. As a side benefit, this adds more call frames that might be helpful with debugging. Testing: tier1
Verified expected inlining on linux-x64 and linux-aarch64 in both release and fastdebug builds.

Lgtm.

Thanks.

albertnetymk · 2020-09-17T09:42:21Z

It also speeds up compilation of that file in fastdebug builds by a noticeable amount.

I see; I believe this is worth mentioning in the comments; sth like these functions should be inlined for performance reasons in release build, and should not be inlined for faster-build reasons in debug build. IOW, this new macro is not only a workaround for compiler bugs but a deliberate choice for certain purposes, which justifies its long-term presence.

The JDK-8253169 failure occurs with gcc9.2, but not with gcc10.2, so may have been fixed already.

I see; thank you for the info.

Increasing the mininum required gcc version for linux-aarch64 to 10.x doesn't seem reasonable at this time, without a much better reason than this.

OK.

mlbridge · 2020-09-17T10:07:07Z

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Sep 17, 2020, at 5:19 AM, Stefan Johansson <sjohanss at openjdk.java.net> wrote:

On Thu, 17 Sep 2020 08:26:06 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

Please review this change to G1 evacuation to prevent inlining of some functions in debug (particularly, fastdebug)
builds. This reduces the size of the generated code to stay away from limits encountered with gcc when compiling for
aarch64. As a side benefit, this adds more call frames that might be helpful with debugging. Testing: tier1
Verified expected inlining on linux-x64 and linux-aarch64 in both release and fastdebug builds.

Looks good.

Thanks.

mlbridge · 2020-09-17T11:31:42Z

Mailing list message from Kim Barrett on hotspot-gc-dev:

On Sep 17, 2020, at 5:45 AM, Albert Mingkun Yang <ayang at openjdk.java.net> wrote:

It also speeds up compilation of that file in fastdebug builds by a noticeable amount.

I see; I believe this is worth mentioning in the comments; sth like these functions should be inlined for performance
reasons in release build, and should *not* be inlined for faster-build reasons in debug build. IOW, this new macro is
not only a workaround for compiler bugs but a deliberate choice for certain purposes, which justifies its long-term
presence.

Done.

albertnetymk

Thank you.

mlbridge · 2020-09-17T11:39:07Z

Mailing list message from Thomas Schatzl on hotspot-gc-dev:

Hi,

On 17.09.20 13:29, Kim Barrett wrote:

On Sep 17, 2020, at 5:45 AM, Albert Mingkun Yang <ayang at openjdk.java.net> wrote:

It also speeds up compilation of that file in fastdebug builds by a noticeable amount.

I see; I believe this is worth mentioning in the comments; sth like these functions should be inlined for performance
reasons in release build, and should *not* be inlined for faster-build reasons in debug build. IOW, this new macro is
not only a workaround for compiler bugs but a deliberate choice for certain purposes, which justifies its long-term
presence.

Done.

still good :)

Thomas

mlbridge · 2020-09-17T11:49:33Z

Mailing list message from Kim Barrett on hotspot-gc-dev:

Thanks.

kstefanj

Still good.

kimbarrett · 2020-09-18T19:28:11Z

/integrate

openjdk · 2020-09-18T19:29:37Z

@kimbarrett Since your change was applied there have been 28 commits pushed to the master branch:

8904420: 8253153: Mentioning of "hour-of-minute" in java.time.temporal.TemporalField JavaDoc
52c28b8: 8253239: Disable VS warning C4307
05572e8: 8253350: Remove unimplemented SharedRuntime::clean_*_entry
43019a0: 8253345: Remove unimplemented Arguments::lookup_logging_aliases
6e9efff: 8253344: Remove unimplemented Arguments::check_gc_consistency
83b2411: 8253342: Fix typos in String.transform
86a1640: 8244505: G1 pause time ratio calculation does not consider Remark/Cleanup pauses
11c4ea9: 8253278: Refactor/cleanup oopDesc::*_klass_addr
73c9088: 8249451: Unconditional exceptions clearing logic in compiler code should honor Async Exceptions.
3ef2efb: 8252721: Nested classes in Swing APIs rely on default constructors
... and 18 more: https://git.openjdk.java.net/jdk/compare/9a7dcdcdbad34e061a8988287fe691abfd4df305...master

Your commit was automatically rebased without conflicts.

Pushed as commit f37c34d.

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

noinline some functions in debug builds

25f8ded

kimbarrett changed the title ~~noinline some functions in debug builds~~ 8253270: Limit fastdebug inlining in G1 evacuation Sep 17, 2020

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Sep 17, 2020

kimbarrett marked this pull request as ready for review September 17, 2020 08:38

openjdk bot added the rfr Pull request is ready for review label Sep 17, 2020

tschatzl approved these changes Sep 17, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 17, 2020

kstefanj approved these changes Sep 17, 2020

View reviewed changes

comment on reduced compile time

0039c4a

albertnetymk approved these changes Sep 17, 2020

View reviewed changes

kstefanj approved these changes Sep 18, 2020

View reviewed changes

openjdk bot closed this Sep 18, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 18, 2020

kimbarrett deleted the limit_inline branch September 18, 2020 20:43

kimbarrett mentioned this pull request Sep 19, 2020

8253237: [REDO] Improve large object handling during evacuation #266

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8253270: Limit fastdebug inlining in G1 evacuation #220

8253270: Limit fastdebug inlining in G1 evacuation #220

kimbarrett commented Sep 17, 2020 •

edited by openjdk bot

bridgekeeper bot commented Sep 17, 2020

openjdk bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020 •

edited

tschatzl left a comment

openjdk bot commented Sep 17, 2020 •

edited

albertnetymk commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

kstefanj left a comment

mlbridge bot commented Sep 17, 2020

albertnetymk commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

albertnetymk left a comment

mlbridge bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

kstefanj left a comment

kimbarrett commented Sep 18, 2020

openjdk bot commented Sep 18, 2020

8253270: Limit fastdebug inlining in G1 evacuation #220

8253270: Limit fastdebug inlining in G1 evacuation #220

Conversation

kimbarrett commented Sep 17, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 17, 2020

openjdk bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020 • edited

Webrevs

tschatzl left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 17, 2020 • edited

albertnetymk commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

kstefanj left a comment

Choose a reason for hiding this comment

mlbridge bot commented Sep 17, 2020

albertnetymk commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

albertnetymk left a comment

Choose a reason for hiding this comment

mlbridge bot commented Sep 17, 2020

mlbridge bot commented Sep 17, 2020

kstefanj left a comment

Choose a reason for hiding this comment

kimbarrett commented Sep 18, 2020

openjdk bot commented Sep 18, 2020

kimbarrett commented Sep 17, 2020 •

edited by openjdk bot

mlbridge bot commented Sep 17, 2020 •

edited

openjdk bot commented Sep 17, 2020 •

edited