Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8262398: Shenandoah: Disable nmethod barrier and stack watermark when running with passive mode #2727

Conversation

zhengyu123
Copy link
Contributor

@zhengyu123 zhengyu123 commented Feb 25, 2021

nmethod barrier and stack watermark allow GC not to process nmethods at GC pauses, and aim to reduce GC latency, they do not benefit STW GCs, who process nmethods at pauses anyway.

Test:

  • hotspot_gc_shenandoah
  • tier1 with -XX:+UseShenandoahGC
  • tier1 with -XX:+UseShenandoahGC -XX:ShenandoahGCMode=passive
  • tier1 with -XX:+UseShenandoahGC -XX:ShenandoahGCMode=passive -XX:-ShenandoahDegeneratedGC

Progress

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

Issue

  • JDK-8262398: Shenandoah: Disable nmethod barrier and stack watermark when running with passive mode

Reviewers

Download

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

To update a local copy of the PR:
$ git checkout pull/2727
$ git pull https://git.openjdk.java.net/jdk pull/2727/head

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 25, 2021

👋 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 openjdk bot added the rfr Pull request is ready for review label Feb 25, 2021
@openjdk
Copy link

openjdk bot commented Feb 25, 2021

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

@openjdk openjdk bot added hotspot-gc hotspot-gc-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Feb 25, 2021
@mlbridge
Copy link

mlbridge bot commented Feb 25, 2021

Webrevs

Copy link
Contributor

@rkennke rkennke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it is possible at all, it would be most useful if we could have flags like ShenandoahNMethodBarrier and ShenandoahStackWatermark, and let them drive enabling and disabling of the corresponding barriers/features. It would be even better, if we could configure concurrent modes such that they do STW class-unloading and stack-processing (if possible with reasonable amount of work). That would help ports where those features are not yet present (current ports under development with the problem are PPC and Graal).

@zhengyu123
Copy link
Contributor Author

If it is possible at all, it would be most useful if we could have flags like ShenandoahNMethodBarrier and ShenandoahStackWatermark, and let them drive enabling and disabling of the corresponding barriers/features. It would be even better, if we could configure concurrent modes such that they do STW class-unloading and stack-processing (if possible with reasonable amount of work). That would help ports where those features are not yet present (current ports under development with the problem are PPC and Graal).

Disabling nmethod barrier and/or stack watermark for concurrent GC means crashes, we no longer have backup (e.g. processing thread roots and code roots at pauses) so I don't think it will help ports.

@zhengyu123 zhengyu123 closed this Mar 16, 2021
@zhengyu123 zhengyu123 reopened this Mar 16, 2021
Copy link
Member

@shipilev shipilev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you. A few minor nits below.

Comment on lines 40 to 46
if (FLAG_IS_DEFAULT(ShenandoahNMethodBarrier)) {
FLAG_SET_DEFAULT(ShenandoahNMethodBarrier, false);
}

if (FLAG_IS_DEFAULT(ShenandoahStackWatermarkBarrier)) {
FLAG_SET_DEFAULT(ShenandoahStackWatermarkBarrier, false);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just go to "// Disable known barriers by default" block below.

@@ -402,7 +402,7 @@ jint ShenandoahHeap::initialize() {
return JNI_OK;
}

void ShenandoahHeap::initialize_heuristics() {
void ShenandoahHeap::initialize_gc_mode() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is initialize_heuristics, which means this should be initialize_mode?

@@ -176,7 +176,7 @@ template <typename IsAlive, typename KeepAlive>
void ShenandoahRootUpdater::roots_do(uint worker_id, IsAlive* is_alive, KeepAlive* keep_alive) {
CodeBlobToOopClosure update_blobs(keep_alive, CodeBlobToOopClosure::FixRelocations);
ShenandoahCodeBlobAndDisarmClosure blobs_and_disarm_Cl(keep_alive);
CodeBlobToOopClosure* codes_cl = ClassUnloading ?
CodeBlobToOopClosure* codes_cl = ClassUnloading && ShenandoahNMethodBarrier ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hunk above has this in parentheses. Pick a style and stick to it, whether parentheses or not.

@@ -179,8 +179,11 @@ class ShenandoahDisarmNMethodsTask : public AbstractGangTask {
};

void ShenandoahCodeRoots::disarm_nmethods() {
ShenandoahDisarmNMethodsTask task;
ShenandoahHeap::heap()->workers()->run_task(&task);
ShenandoahHeap* const heap = ShenandoahHeap::heap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This heap getter can be moved into the block below, and maybe even inlined?

@openjdk
Copy link

openjdk bot commented Mar 16, 2021

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

8262398: Shenandoah: Disable nmethod barrier and stack watermark when running with passive mode

Reviewed-by: rkennke, 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 39 new commits pushed to the master branch:

  • 000012a: 8148937: (str) Adapt StringJoiner for Compact Strings
  • a707fcb: 8263723: [BACKOUT] MoveAndUpdateClosure::do_addr calls function with side-effects in an assert
  • 86e9cd9: 8263667: Avoid running GitHub actions on branches named pr/*
  • 41276eb: 8259863: doc: JShell snippet doesn't compile
  • f9f2eef: 8263434: Dangling references after MethodComparator::methods_EMCP
  • 23fc2a4: 8263688: Coordinate equals, hashCode and compareTo of JavacFileManager.PathAndContainer
  • d1baed6: 8263672: fatal error: no reachable node should have no use
  • 086a66a: 8261095: Add test for clhsdb "symbol" command
  • ec95a5c: 8263410: ListModel javadoc refers to non-existent interface
  • 7b9d256: 8261262: Kitchensink24HStress.java crashed with EXCEPTION_ACCESS_VIOLATION
  • ... and 29 more: https://git.openjdk.java.net/jdk/compare/4f1cda4fd744ca159782c09e9c8098f3aa196e72...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 16, 2021
@shipilev
Copy link
Member

Forgot one thing. These tests might also need modification to test that we never disable this for true heuristics, etc.

./test/hotspot/jtreg/gc/shenandoah/options/TestSelectiveBarrierFlags.java
./test/hotspot/jtreg/gc/shenandoah/options/TestWrongBarrierEnable.java
./test/hotspot/jtreg/gc/shenandoah/options/TestWrongBarrierDisable.java

Comment on lines 49 to 50
new String[] { "ShenandoahNMethodBarrier"},
new String[] { "ShenandoahStackWatermarkBarrier"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note there is a space between " and }. Minor nit, fix before push.

Copy link
Contributor

@rkennke rkennke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks!

@zhengyu123
Copy link
Contributor Author

/integrate

@openjdk openjdk bot closed this Mar 17, 2021
@openjdk 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 Mar 17, 2021
@openjdk
Copy link

openjdk bot commented Mar 17, 2021

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

  • 4f4ca0e: 8261671: X86 I2L conversion can be skipped for certain masked positive values
  • 5d87a21: 8263361: Incorrect arraycopy stub selected by C2 for SATB collectors
  • e152cc0: 8263677: Improve Character.isLowerCase/isUpperCase lookups
  • b63b5d4: 8263732: ProblemList serviceability/sa/ClhsdbSymbol.java on ZGC
  • 000012a: 8148937: (str) Adapt StringJoiner for Compact Strings
  • a707fcb: 8263723: [BACKOUT] MoveAndUpdateClosure::do_addr calls function with side-effects in an assert
  • 86e9cd9: 8263667: Avoid running GitHub actions on branches named pr/*
  • 41276eb: 8259863: doc: JShell snippet doesn't compile
  • f9f2eef: 8263434: Dangling references after MethodComparator::methods_EMCP
  • 23fc2a4: 8263688: Coordinate equals, hashCode and compareTo of JavacFileManager.PathAndContainer
  • ... and 33 more: https://git.openjdk.java.net/jdk/compare/4f1cda4fd744ca159782c09e9c8098f3aa196e72...master

Your commit was automatically rebased without conflicts.

Pushed as commit 7674da4.

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

@zhengyu123 zhengyu123 deleted the JDK-8262398-disable-barriers-stw-gc branch May 2, 2022 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated shenandoah shenandoah-dev@openjdk.org
3 participants