8286829: Shenandoah: fix Shenandoah Loom support #8924

zhengyu123 · 2022-05-27T14:52:19Z

Please review the patch that fixes Loom support in Shenandoah GC.

stackChunkOop is a special oop that contains stack metadata, we need to utilize nmethod entry barrier to mark, evacuate and update stack metadata.
During evacuation and reference updating phase, we can not guarantee all metadata in stackChunkOop is deeply good, so I force it to take slow path for the correctness, will try to optimize it later in separate CR.
Shenandoah uses similar way to arm and disarm nmethod as the new mechanism introduced in JDK-8284161. We may want to migrate to it in followup CR.

Test:

tier1 with ShenandoahGC (Linux x86_64 and Windows x64)
tier2 with Shenandoah GC (Linux x86_64 and Windows x64)
hotspot_gc_shenandoah on Linux x86_32
tier1 with ShenandoahGC + Loom (Linux x86_64 and Windows x64)
tier2 with ShenandoahGC + Loom (Linux x86_64 and Windows x64)

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-8286829: Shenandoah: fix Shenandoah Loom support

Reviewers

Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8924/head:pull/8924
$ git checkout pull/8924

Update a local copy of the PR:
$ git checkout pull/8924
$ git pull https://git.openjdk.java.net/jdk pull/8924/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8924

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8924.diff

…error message

bridgekeeper · 2022-05-27T14:53:21Z

👋 Welcome back zgu! 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-05-27T14:55:35Z

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

hotspot
shenandoah

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 · 2022-05-27T23:21:56Z

Webrevs

shipilev · 2022-05-30T07:06:29Z

src/hotspot/share/gc/shenandoah/shenandoahConcurrentGC.cpp


  heap->set_concurrent_mark_in_progress(true);

+  _mark.start_mark();


Style: call start_mark() here maybe? Matches the use of end_mark() elsewhere.

Okay. I am award of asymmetry, but not sure this is better.

src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp

shipilev

I am okay with most of it, but still concerned with the injecting new code into hot marking loops.

src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp

shipilev

I still have a question: now that closures drop do_metadata, does that mean we always traverse metadata now? Which means we can remove half of the closures? For example, ShenandoahMarkUpdateRefsClosure and ShenandoahMarkUpdateRefsMetadataClosure are pretty much the same, right? We can leave ShenandoahMarkUpdateRefsClosure only then?

zhengyu123 · 2022-06-01T15:56:52Z

ShenandoahMarkUpdateRefsMetadataClosure

Yes, we always walk metadata now.

And right, I need to look into duplicated closures. But can we do that in separate CR? Another round of tests going to take a couple of days, and I want to get this in before RDP1.

Thanks.

shipilev

All right then!

openjdk · 2022-06-01T16:12:04Z

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

8286829: Shenandoah: fix Shenandoah Loom support

Reviewed-by: shade

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

67ecd30: 8287398: Allow concurrent execution of hotspot docker tests
8071b23: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux
774928f: 8287625: ProblemList jdk/jshell/HighlightUITest.java on all platforms
e3791ec: 8287491: compiler/jvmci/errors/TestInvalidDebugInfo.java fails new assert: assert((uint)t < T_CONFLICT + 1) failed: invalid type #
f8eb7a8: 8287512: continuationEntry.hpp has incomplete definitions
b2b4ee2: 8287233: Crash in Continuation.enterSpecial: stop: tried to execute native method as non-native
168b226: 8282662: Use List.of() factory method to reduce memory consumption
48f19e4: 8287453: RenderPerfTest incorrectly measures performance
0ef3d85: 8287552: riscv: Fix comment typo in li64
e0e15de: 8287366: Improve test failure reporting in GHA
... and 89 more: https://git.openjdk.java.net/jdk/compare/6cc4bb1169f34bc091cad3e2deec37cd5585e8d5...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.

zhengyu123 · 2022-06-01T17:00:06Z

Thanks, @shipilev

zhengyu123 · 2022-06-01T17:00:15Z

/integrate

openjdk · 2022-06-01T17:01:34Z

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

67ecd30: 8287398: Allow concurrent execution of hotspot docker tests
8071b23: 8287237: (fs) Files.probeContentType returns null if filename contains hash mark on Linux
774928f: 8287625: ProblemList jdk/jshell/HighlightUITest.java on all platforms
e3791ec: 8287491: compiler/jvmci/errors/TestInvalidDebugInfo.java fails new assert: assert((uint)t < T_CONFLICT + 1) failed: invalid type #
f8eb7a8: 8287512: continuationEntry.hpp has incomplete definitions
b2b4ee2: 8287233: Crash in Continuation.enterSpecial: stop: tried to execute native method as non-native
168b226: 8282662: Use List.of() factory method to reduce memory consumption
48f19e4: 8287453: RenderPerfTest incorrectly measures performance
0ef3d85: 8287552: riscv: Fix comment typo in li64
e0e15de: 8287366: Improve test failure reporting in GHA
... and 89 more: https://git.openjdk.java.net/jdk/compare/6cc4bb1169f34bc091cad3e2deec37cd5585e8d5...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-06-01T17:01:53Z

@zhengyu123 Pushed as commit 239ac2a.

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

Zhengyu Gu added 24 commits May 23, 2022 17:52

v0

1249cbd

8287090: Shenandoah: after updaterefs verification reports confusing …

ca26029

…error message

v1

c3bc7ae

v0

aef730b

v1

370c998

v2

c067317

v3

177b70f

v3

79d56b0

comments

baac648

Fix marking cycle

d96a25e

More fixes on marking closures

70bf7b2

Merge branch 'master' into JDK-8286829-mark-vthread

89b9873

v4

c6e8cf9

v5

9858934

cleanup

a3603c1

cleanup and update copyright years

95dc432

Fix stw mark

7e244c5

fix

993c602

Fix requires_barriers

fcb0ce1

Simplify

f13a40b

Fix

21ce33e

Cleanup

cc95dd1

Fix comment

33b2f95

Fix comment

7db9c0b

openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels May 27, 2022

zhengyu123 marked this pull request as ready for review May 27, 2022 23:16

openjdk bot added the rfr Pull request is ready for review label May 27, 2022

shipilev reviewed May 30, 2022

View reviewed changes

Zhengyu Gu added 3 commits May 31, 2022 09:12

Aleksey's comment

b86b51b

NMethod::heal_nmethod() does not need to handle ref-update

4186c03

Fix

ed84a9e

shipilev reviewed Jun 1, 2022

View reviewed changes

src/hotspot/share/gc/shenandoah/shenandoahMark.inline.hpp Show resolved Hide resolved

shipilev reviewed Jun 1, 2022

View reviewed changes

shipilev approved these changes Jun 1, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 1, 2022

openjdk bot added the integrated Pull request has been integrated label Jun 1, 2022

openjdk bot closed this Jun 1, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 1, 2022


		heap->set_concurrent_mark_in_progress(true);

		_mark.start_mark();

8286829: Shenandoah: fix Shenandoah Loom support #8924

8286829: Shenandoah: fix Shenandoah Loom support #8924

Uh oh!

Conversation

zhengyu123 commented May 27, 2022 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 27, 2022

Uh oh!

openjdk bot commented May 27, 2022

Uh oh!

mlbridge bot commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

shipilev May 30, 2022

Choose a reason for hiding this comment

Uh oh!

zhengyu123 May 31, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

zhengyu123 commented Jun 1, 2022

Uh oh!

shipilev left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 1, 2022

Uh oh!

zhengyu123 commented Jun 1, 2022

Uh oh!

zhengyu123 commented Jun 1, 2022

Uh oh!

openjdk bot commented Jun 1, 2022

Uh oh!

openjdk bot commented Jun 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

zhengyu123 commented May 27, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented May 27, 2022 •

edited

Loading