8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack #11314

reinrich · 2022-11-23T10:05:56Z

This pr removes the stackwalks to keep alive oops of nmethods found on stack during G1 remark as it seems redundant. The oops are already kept alive by the nmethod entry barrier

Additionally it fixes a comment that says nmethod entry barriers are needed to deal with continuations which, afaik, is not the case. Please correct me and explain if I'm mistaken.

Testing: the patch is included in our daily CI testing since a week. That is most JCK and JTREG tests, also in Xcomp mode, Renaissance benchmark and SAP specific tests with fastdebug and release builds on the standard platforms plus PPC64. There was no failure I could attribute to this change.

I tried to find a jtreg test that is sensitive to the keep alive by omitting it in the nmethod entry barrier and also in G1 remark but without success.

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-8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack

Reviewers

Thomas Schatzl (@tschatzl - Reviewer) ⚠️ Review applies to 37ee165b
Albert Mingkun Yang (@albertnetymk - Reviewer) ⚠️ Review applies to 37ee165b
Erik Österlund (@fisk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11314

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

Using diff file

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

bridgekeeper · 2022-11-23T10:07:25Z

👋 Welcome back rrich! 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 · 2022-11-23T10:10:32Z

@reinrich The following label will be automatically applied to this pull request:

hotspot

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 pull request command.

reinrich · 2022-11-23T10:54:44Z

/label hotspot-compiler,hotspot-gc

reinrich · 2022-11-23T10:55:00Z

/label remove hotspot

openjdk · 2022-11-23T10:55:23Z

@reinrich
The hotspot-compiler label was successfully added.

The hotspot-gc label was successfully added.

openjdk · 2022-11-23T10:57:18Z

@reinrich
The hotspot label was successfully removed.

mlbridge · 2022-11-23T14:00:04Z

Webrevs

01: Full (54f2e80d)
00: Full (37ee165b)

tschatzl

Lgtm. Will push it through our testing.

openjdk · 2022-11-25T11:30:46Z

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

8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack

Reviewed-by: tschatzl, ayang, eosterlund

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 6 new commits pushed to the master branch:

66db0bb: 8299692: G1: Remove unused G1BlockOffsetTable::is_card_boundary
7068457: 8298472: AArch64: Detect Ampere-1 and Ampere-1A CPUs and set default options
500c3c1: 8298730: Refactor subsystem_file_line_contents and add docs and tests
4072412: 8298876: Swing applications do not get repainted coming out of sleep on Windows 10
a503ec2: 8299608: Add Register + imm32 orq to x86_64 assembler
d2827ec: 8299671: speed up compiler/intrinsics/string/TestStringLatin1IndexOfChar.java

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

reinrich · 2022-11-25T13:52:52Z

Lgtm. Will push it through our testing.

Thanks!

tschatzl · 2022-11-25T15:21:08Z

One other note: since the ARM32 port does not have this nmethod walk safety net during Remark pause anymore and it does not implement nmethod barriers (at least that's what that nullptr return value by that removed comment indicates to me) it may be good to talk to ARM32 maintainers about this change. I do not know who maintains ARM32 code.

We at Oracle do not support ARM32 so it should be good, but it may help ARM32 maintainers to keep this now removed code after all.

reinrich · 2022-11-25T16:12:53Z

One other note: since the ARM32 port does not have this nmethod walk safety net during Remark pause anymore and it does not implement nmethod barriers (at least that's what that nullptr return value by that removed comment indicates to me) it may be good to talk to ARM32 maintainers about this change. I do not know who maintains ARM32 code.

We at Oracle do not support ARM32 so it should be good, but it may help ARM32 maintainers to keep this now removed code after all.

Good point. It is myy understanding (also stated in the JBS item) that G1 concurrent marking requires the keep alive of oop constants by the nmethod entry barriers for SATB correctness. So without the entry barriers ARM32 has an issue there already because the keep alive during the remark pause is not sufficient, is it?

There's a JBS item JDK-8291302 to implement the nmethod entry barrier assigned to @bulasevich. Boris are there plans to implement the nmethod entry barriers on ARM32 in the near future given that they are required for G1 correctness?

tschatzl · 2022-11-25T19:25:30Z

We at Oracle do not support ARM32 so it should be good, but it may help ARM32 maintainers to keep this now removed code after all.

Good point. It is myy understanding (also stated in the JBS item) that G1 concurrent marking requires the keep alive of oop constants by the nmethod entry barriers for SATB correctness. So without the entry barriers ARM32 has an issue there already because the keep alive during the remark pause is not sufficient, is it?

That is true, but it might make the problem larger than necessary - although admittedly, the comments indicate some magic hand-waving of the effectiveness of this additional walk through the nmethods on thread stacks.

Imho G1 has worked well enough with that level of wrongness for a long time on the other platforms, so keeping this (little amount of) code may help ARM32 maintainers to tide over a little bit (i.e. not make their platforms potentially crash left and right) until they are ready with their nmethod barrier implementation.

I'm good either way you choose.

reinrich · 2022-11-28T14:56:25Z

I would like to get rid of the stackwalks if they are unnecessary, for pause time reduction but also to ease understanding the code. I'm not in a hurry though.
I've posted on the porters list to get some feedback from ARM32 maintainers: https://mail.openjdk.org/pipermail/porters-dev/2022-November/000739.html

reinrich · 2022-11-30T15:10:15Z

Thanks for the review @albertnetymk

bridgekeeper · 2022-12-28T16:31:56Z

@reinrich This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

TheRealMDoerr · 2023-01-03T12:03:53Z

ARM32 issue is solved: 245f0cf

fisk

Looks good.

reinrich · 2023-01-09T12:54:22Z

Looks good.

Thank you Erik. I'll integrate this after another round of tests.

reinrich · 2023-01-10T10:31:10Z

/integrate

openjdk · 2023-01-10T10:32:45Z

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

c8a8388: 8299789: Compilation of gtest causes build to fail if runtime libraries are in different dirs
5f37cbe: 8297572: Remove unused PrecisionStyle::Precise
f95346e: 8299261: Clean up AWT D3D exports
195f313: 8287925: AArch64: intrinsics for compareUnsigned method in Integer and Long
3a66737: 8299525: RISC-V: Add backend support for half float conversion intrinsics
b7eb4e2: 8297306: Incorrect brackets in Javadoc for a constructor of IteratorSpliterator
a638663: 8255119: Monitor::wait takes signed integer as timeout
4395578: Merge
d49851a: 8299689: Make use of JLine for Console as "opt-in"
f79b3d4: 6381945: (cal) Japanese calendar unit test system should avoid multiple static imports
... and 13 more: https://git.openjdk.org/jdk/compare/05a0a710313917fe7124ff43fe9c9af1d649bcac...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-10T10:33:03Z

@reinrich Pushed as commit eab1e62.

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

G1 Remark: no need to keep alive const oops for nmethods on stack

37ee165

openjdk bot added the hotspot hotspot-dev@openjdk.org label Nov 23, 2022

openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org hotspot-gc hotspot-gc-dev@openjdk.org labels Nov 23, 2022

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Nov 23, 2022

reinrich marked this pull request as ready for review November 23, 2022 13:53

openjdk bot added the rfr Pull request is ready for review label Nov 23, 2022

tschatzl approved these changes Nov 25, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 25, 2022

albertnetymk approved these changes Nov 30, 2022

View reviewed changes

Merge branch 'master'

54f2e80

fisk approved these changes Jan 9, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 10, 2023

openjdk bot closed this Jan 10, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 10, 2023

reinrich deleted the 8297487_g1_remark_nmethod_keepalive_is_redundant branch February 20, 2023 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack #11314

8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack #11314

reinrich commented Nov 23, 2022 •

edited by openjdk bot

bridgekeeper bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

reinrich commented Nov 23, 2022

reinrich commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

mlbridge bot commented Nov 23, 2022 •

edited

tschatzl left a comment •

edited

openjdk bot commented Nov 25, 2022 •

edited

reinrich commented Nov 25, 2022

tschatzl commented Nov 25, 2022 •

edited

reinrich commented Nov 25, 2022

tschatzl commented Nov 25, 2022

reinrich commented Nov 28, 2022 •

edited

reinrich commented Nov 30, 2022

bridgekeeper bot commented Dec 28, 2022

TheRealMDoerr commented Jan 3, 2023

fisk left a comment

reinrich commented Jan 9, 2023

reinrich commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack #11314

8297487: G1 Remark: no need to keep alive oop constants of nmethods on stack #11314

Conversation

reinrich commented Nov 23, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

reinrich commented Nov 23, 2022

reinrich commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

openjdk bot commented Nov 23, 2022

mlbridge bot commented Nov 23, 2022 • edited

Webrevs

tschatzl left a comment • edited

Choose a reason for hiding this comment

openjdk bot commented Nov 25, 2022 • edited

reinrich commented Nov 25, 2022

tschatzl commented Nov 25, 2022 • edited

reinrich commented Nov 25, 2022

tschatzl commented Nov 25, 2022

reinrich commented Nov 28, 2022 • edited

reinrich commented Nov 30, 2022

bridgekeeper bot commented Dec 28, 2022

TheRealMDoerr commented Jan 3, 2023

fisk left a comment

Choose a reason for hiding this comment

reinrich commented Jan 9, 2023

reinrich commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

reinrich commented Nov 23, 2022 •

edited by openjdk bot

mlbridge bot commented Nov 23, 2022 •

edited

tschatzl left a comment •

edited

openjdk bot commented Nov 25, 2022 •

edited

tschatzl commented Nov 25, 2022 •

edited

reinrich commented Nov 28, 2022 •

edited