8253434: Shenandoah: Cleanup ShenandoahRootScanner #286

zhengyu123 · 2020-09-21T18:11:10Z

After moving CLDG mark to concurrent phase, ShenandoahRootScanner::strong_roots_do() and ShenandoahRootScanner::roots_do() are practical the same, let's trim one.

Test:
hotspot_gc_shenandoah

Progress

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

Issue

JDK-8253434: Shenandoah: Cleanup ShenandoahRootScanner

Reviewers

Roman Kennke (@rkennke - Reviewer) ⚠️ Review applies to 2b40913

Download

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

bridgekeeper · 2020-09-21T18:12:26Z

👋 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 · 2020-09-21T18:14:20Z

@zhengyu123 The following labels will be automatically applied to this pull request: hotspot-gc 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 (add|remove) "label" command.

mlbridge · 2020-09-21T18:18:57Z

Webrevs

rkennke

It looks to me you can also trim out the CLD closures altogether.

rkennke · 2020-09-21T18:25:04Z

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

@@ -189,36 +189,13 @@ ShenandoahRootScanner::~ShenandoahRootScanner() {
 }

 void ShenandoahRootScanner::roots_do(uint worker_id, OopClosure* oops) {
-  CLDToOopClosure clds_cl(oops, ClassLoaderData::_claim_strong);
+  CLDToOopClosure clds_cl(oops, ClassLoaderData::_claim_none);


Are you sure that we don't have to claim CLDs? How would the threads avoid doubly-marking CLDs?

Actually it looks like the CLD closure is not used at all, or is it?

Right, we don't need to mark on-stack CLDs, concurrent mark CLDs should be sufficient.

Reran tests, still good.

rkennke · 2020-09-22T10:07:25Z

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

-  CLDToOopClosure clds_cl(oops, ClassLoaderData::_claim_strong);
-  MarkingCodeBlobClosure blobs_cl(oops, !CodeBlobToOopClosure::FixRelocations);
-  strong_roots_do(worker_id, oops, &clds_cl, &blobs_cl);
+  roots_do(worker_id, oops, NULL, &blobs_cl);
 }

 void ShenandoahRootScanner::roots_do(uint worker_id, OopClosure* oops, CLDClosure* clds, CodeBlobClosure* code, ThreadClosure *tc) {


You're still passing a (NULL) CLDClosure here, but it's not used. Or am I missing something? The method argument could be eliminated altogether.

It is unused, removed.

Thanks

openjdk · 2020-09-22T10:08:21Z

@zhengyu123 this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8253434
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

rkennke

Looks good to me now! Thank you!

zhengyu123 · 2020-09-22T13:01:22Z

/integrate

openjdk · 2020-09-22T13:02:17Z

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

8c02bdb: 8252921: NMT overwrite memory type for region assert when building dynamic archive
0c287de: 8253421: Initialize JFR trace-IDs with zero
aa38624: 8248352: [TEST_BUG] Test test/jdk/java/awt/font/TextLayout/ArabicDiacriticTest.java can leave frame open
ae20dd6: 8251496: Fix doclint warnings in jdk.net.httpserver
b9729cb: 8253284: Zero OrderAccess barrier mappings are incorrect
284bbf0: 8253079: DeterministicDump.java fails due to garbage in structure padding
a4affd5: 8253412: Unsupported GC options passed in JAVA_TOOL_OPTIONS are silently ignored
96f722c: 8252114: Windows-AArch64: Enable and test ZGC and ShenandoahGC
f7b1ce4: 8253447: Remove buggy code introduced by 8249451
282b9dc: 8081833: Clean up JVMFlag getter/setter code
... and 25 more: https://git.openjdk.java.net/jdk/compare/d35b117956cff96bd918f2dd10cdf71829e5c525...master

It was not possible to rebase your changes automatically. Please merge master into your branch and try again.

openjdk · 2020-09-22T14:00:09Z

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

8253434: Shenandoah: Cleanup ShenandoahRootScanner

Reviewed-by: rkennke

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.

There are currently no new commits on the master branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate 8c02bdbf13bc8258bbdb6c45fd3b3ba4c6f8971b.

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

zhengyu123 · 2020-09-22T14:02:33Z

/integrate

openjdk · 2020-09-22T14:03:36Z

@zhengyu123 Pushed as commit 3d5fea1.

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

Trim ShenandoahRootScanner

a6d797c

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

openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Sep 21, 2020

rkennke reviewed Sep 21, 2020

View reviewed changes

No need to mark on-stack CLD

66492e4

rkennke reviewed Sep 22, 2020

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 22, 2020

Remove unused CLD closure

2b40913

rkennke approved these changes Sep 22, 2020

View reviewed changes

Merge master

78afc92

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Sep 22, 2020

openjdk bot closed this Sep 22, 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 22, 2020

zhengyu123 deleted the JDK-8253434 branch September 22, 2020 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8253434: Shenandoah: Cleanup ShenandoahRootScanner #286

8253434: Shenandoah: Cleanup ShenandoahRootScanner #286

zhengyu123 commented Sep 21, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

mlbridge bot commented Sep 21, 2020 •

edited

Loading

rkennke left a comment

rkennke Sep 21, 2020

rkennke Sep 21, 2020

zhengyu123 Sep 21, 2020

rkennke Sep 22, 2020

zhengyu123 Sep 22, 2020

openjdk bot commented Sep 22, 2020

rkennke left a comment

zhengyu123 commented Sep 22, 2020

openjdk bot commented Sep 22, 2020

openjdk bot commented Sep 22, 2020

zhengyu123 commented Sep 22, 2020

openjdk bot commented Sep 22, 2020

8253434: Shenandoah: Cleanup ShenandoahRootScanner #286

8253434: Shenandoah: Cleanup ShenandoahRootScanner #286

Conversation

zhengyu123 commented Sep 21, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

mlbridge bot commented Sep 21, 2020 • edited Loading

Webrevs

rkennke left a comment

Choose a reason for hiding this comment

rkennke Sep 21, 2020

Choose a reason for hiding this comment

rkennke Sep 21, 2020

Choose a reason for hiding this comment

zhengyu123 Sep 21, 2020

Choose a reason for hiding this comment

rkennke Sep 22, 2020

Choose a reason for hiding this comment

zhengyu123 Sep 22, 2020

Choose a reason for hiding this comment

openjdk bot commented Sep 22, 2020

rkennke left a comment

Choose a reason for hiding this comment

zhengyu123 commented Sep 22, 2020

openjdk bot commented Sep 22, 2020

openjdk bot commented Sep 22, 2020

zhengyu123 commented Sep 22, 2020

openjdk bot commented Sep 22, 2020

zhengyu123 commented Sep 21, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 21, 2020 •

edited

Loading