8325872: Make GuaranteedSafepointInterval default 0 #18820

coleenp · 2024-04-17T16:03:57Z

This change keeps the GuaranteedSafepointInterval option for use with -XX:+HandshakeALot and -XX:+SafepointALot and removes it from working with the MonitorDeflationThread. The MonitorDeflation thread has it's own timers and doesn't require safepoint to trigger anymore, so the GuaranteedSafepointInterval is logically unrelated except for historical reasons.
The option AsyncDeflationInterval default is 250 so was used anyway in this code for the monitor deflation thread timer.

Tested with tier1-8. Also performance tested this which didn't show any improvements.

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-8325872: Make GuaranteedSafepointInterval default 0 (Enhancement - P4)

Reviewers

Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to d79b7616
Erik Österlund (@fisk - Reviewer) ⚠️ Review applies to 4501dc02
Daniel D. Daugherty (@dcubed-ojdk - Reviewer) ⚠️ Review applies to 4501dc02

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18820

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-17T16:04:23Z

👋 Welcome back coleenp! 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 · 2024-04-17T16:04:24Z

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

8325872: Make GuaranteedSafepointInterval default 0

Reviewed-by: shade, eosterlund, dcubed

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

0646284: 8317376: Minor improvements to the 'this' escape analyzer
4895a15: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object
fd331ff: 8325469: Freeze/Thaw code can crash in the presence of OSR frames
9fd7802: 8325494: C2: Broken graph after not skipping CastII node anymore for Assertion Predicates after JDK-8309902
192ec38: 8329595: spurious variable "might not have been initialized" on static final field
03e8417: 8329948: Remove string template feature
ff3e76f: 8330053: JFR: Use LocalDateTime instead ZonedDateTime
811aadd: 8324683: Unify AttachListener code for Posix platforms
5841cb3: 8330107: Separate out "awt" libraries from Awt2dLibraries.gmk
89129e3: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant
... and 42 more: https://git.openjdk.org/jdk/compare/60d88b7ae2945724ab4db44207e3390bcff172c0...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 · 2024-04-17T16:05:07Z

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

hotspot-runtime

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.

mlbridge · 2024-04-17T16:08:13Z

Webrevs

shipilev

Good riddance.

fisk · 2024-04-17T16:20:33Z

src/hotspot/share/runtime/arguments.cpp

@@ -3645,6 +3645,11 @@ jint Arguments::parse(const JavaVMInitArgs* initial_cmd_args) {

  apply_debugger_ergo();

+  // The VMThread needs to stop now and then to execute these debug options.
+  if (HandshakeALot && SafepointALot && FLAG_IS_DEFAULT(GuaranteedSafepointInterval)) {


Should this be (HandshakeALot || SafepointALot) instead of HandshakeALot && SafepointALot?

Yes it should be.

…keALot.

fisk

Looks good!

dcubed-ojdk

Thumbs up.

I only had a question about some code that you might want to change.

dcubed-ojdk · 2024-04-17T21:09:51Z

src/hotspot/share/runtime/vmThread.cpp

    return false;
  }
  static jlong last_halot_ms = 0;


Below this point is this code:

// If only HandshakeALot is set, but GuaranteedSafepointInterval is 0, // we emit a handshake if it's been more than a second since the last one. jlong interval = GuaranteedSafepointInterval != 0 ? GuaranteedSafepointInterval : 1000;

Do you need this now that you've made the change in src/hotspot/share/runtime/arguments.cpp?

You're right. This has strange logic. We only set GuaranteedSafepointInterval in arguments.cpp if the flag hasn't been set on the command line. So someone could set explicitly set GuaranteeSafepointInterval=0. If GuaranteedSafepointInterval is 0 then we can get to this code if another safepoint operation wakes up the VM Thread. And in this case, we want to make sure it's been longer than a second ago.

I fixed the comment and changed the variable name to alot_ms.

coleenp · 2024-04-18T11:21:03Z

Thanks for the reviews and comments, Aleksey, Erik and Dan.
/integrate

openjdk · 2024-04-18T11:21:41Z

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

b049609: 8330094: RISC-V: Save and restore FRM in the call stub
4b55fe5: 8330520: linux clang build fails in os_linux.cpp with static_assert with no message is a C++17 extension
5eb2c59: 8330475: Remove unused default value for ModRefBarrierSet::write_ref_array_pre
003e86f: 8324755: Enable parallelism in vmTestbase/gc/gctests/LargeObjects tests
706b421: 8330467: NoClassDefFoundError when lambda is in a hidden class
fe0227e: 8311098: Change comment in verificationType.hpp to refer to _sym
0646284: 8317376: Minor improvements to the 'this' escape analyzer
4895a15: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object
fd331ff: 8325469: Freeze/Thaw code can crash in the presence of OSR frames
9fd7802: 8325494: C2: Broken graph after not skipping CastII node anymore for Assertion Predicates after JDK-8309902
... and 48 more: https://git.openjdk.org/jdk/compare/60d88b7ae2945724ab4db44207e3390bcff172c0...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-18T11:21:46Z

@coleenp Pushed as commit 60b65e6.

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

8325872: Make GuaranteedSafepointInterval default 0

d79b761

openjdk bot added the rfr Pull request is ready for review label Apr 17, 2024

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Apr 17, 2024

shipilev approved these changes Apr 17, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 17, 2024

fisk suggested changes Apr 17, 2024

View reviewed changes

Fix argument conditionals, reran tests with SafepointALot and Handsha…

4501dc0

…keALot.

fisk approved these changes Apr 17, 2024

View reviewed changes

dcubed-ojdk approved these changes Apr 17, 2024

View reviewed changes

Update handshake_or_safepoint_alot comment.

2f1a767

openjdk bot added the integrated Pull request has been integrated label Apr 18, 2024

openjdk bot closed this Apr 18, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 18, 2024

coleenp deleted the safepoint-stop branch April 18, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8325872: Make GuaranteedSafepointInterval default 0 #18820

8325872: Make GuaranteedSafepointInterval default 0 #18820

coleenp commented Apr 17, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 17, 2024

openjdk bot commented Apr 17, 2024 •

edited

Loading

openjdk bot commented Apr 17, 2024

mlbridge bot commented Apr 17, 2024 •

edited

Loading

shipilev left a comment

fisk Apr 17, 2024

coleenp Apr 17, 2024

fisk left a comment

dcubed-ojdk left a comment

dcubed-ojdk Apr 17, 2024

coleenp Apr 17, 2024

coleenp commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

8325872: Make GuaranteedSafepointInterval default 0 #18820

8325872: Make GuaranteedSafepointInterval default 0 #18820

Conversation

coleenp commented Apr 17, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 17, 2024

openjdk bot commented Apr 17, 2024 • edited Loading

openjdk bot commented Apr 17, 2024

mlbridge bot commented Apr 17, 2024 • edited Loading

Webrevs

shipilev left a comment

Choose a reason for hiding this comment

fisk Apr 17, 2024

Choose a reason for hiding this comment

coleenp Apr 17, 2024

Choose a reason for hiding this comment

fisk left a comment

Choose a reason for hiding this comment

dcubed-ojdk left a comment

Choose a reason for hiding this comment

dcubed-ojdk Apr 17, 2024

Choose a reason for hiding this comment

coleenp Apr 17, 2024

Choose a reason for hiding this comment

coleenp commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

openjdk bot commented Apr 18, 2024

coleenp commented Apr 17, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 17, 2024 •

edited

Loading

mlbridge bot commented Apr 17, 2024 •

edited

Loading