8301819: Enable continuations code by default #12459

AlanBateman · 2023-02-07T18:37:03Z

The continuations code added in JDK 19 with JEP 425 is currently disabled by default and is only enabled when running with preview features enabled (--enable-preview). Disabled by default was the pragmatic choice at that time to reduce risk and possible performance regressions. We'd like to change this so the code is enabled by default on the platforms that have a port. Enabling this code by default helps to shake out any issues early in JDK 21 and sets things up to make virtual threads a permanent feature in the future. If something comes up in the next few weeks/months then it can be changed to be disabled by default again. This change has no impact on the builds that do not have a port.

Testing: tier1-tier6

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-8301819: Enable continuations code by default

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 2ef27802
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 2ef27802
Daniel D. Daugherty (@dcubed-ojdk - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12459

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

Using diff file

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

bridgekeeper · 2023-02-07T18:38:27Z

👋 Welcome back alanb! 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 · 2023-02-07T18:40:31Z

@AlanBateman 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.

AlanBateman · 2023-02-08T12:57:20Z

/label remove hotspot-runtime
/label add hotspot

openjdk · 2023-02-08T12:58:24Z

@AlanBateman
The hotspot-runtime label was successfully removed.

openjdk · 2023-02-08T12:59:35Z

@AlanBateman
The hotspot label was successfully added.

mlbridge · 2023-02-08T15:17:23Z

Webrevs

vnkozlov · 2023-02-08T20:00:45Z

@AlanBateman Do you have performance data for our regular benchmarks?

vnkozlov

Alan showed me performance data with nothing alarming (at least from benchmarks we use).
I agree with these changes.

openjdk · 2023-02-08T20:32:26Z

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

8301819: Enable continuations code by default

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

4539899: 8301480: Replace NULL with nullptr in os/posix
1428db7: 8301224: Replace NULL with nullptr in share/gc/shared/
c25b4f4: 8301578: Perform output outside synchronization in Module.class
5830c03: 8302004: InlineTree should consult replay file in release build
c8ace48: 8301072: Replace NULL with nullptr in share/oops/
1c7b09b: 8302069: javax/management/remote/mandatory/notif/NotifReconnectDeadlockTest.java update
837d464: 8302125: Make G1 full gc abort the VM after failing VerifyDuringGC verification

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.

dcubed-ojdk · 2023-02-08T20:44:05Z

@AlanBateman - Thanks for documenting that Tier[1-6] testing was done on this change.

dholmes-ora

@AlanBateman I agree with enabling this and giving it time to bake. Vladimir has already checked there are no red flags on the performance front, so that is fine. If anything does arise we have, as you note, time to address it.

Copyright year needs updating in continuation.cpp.

Thanks.

AlanBateman · 2023-02-09T17:07:19Z

@AlanBateman I agree with enabling this and giving it time to bake. Vladimir has already checked there are no red flags on the performance front, so that is fine. If anything does arise we have, as you note, time to address it.

@kuksenko did a lot of detailed analysis on the performance, this lead to JDK-8300002 to account for the interaction between inlining and the post-call nops. We can re-visit again in a few weeks or months if something comes up.

Copyright year needs updating in continuation.cpp.

Yes, will do.

shqking · 2023-02-10T03:52:30Z

src/hotspot/share/runtime/continuation.cpp

 bool Continuations::enabled() {
-  return VMContinuations && Arguments::enable_preview();


With this change, I think header file "runtime/arguments.hpp" at line 29 can be excluded.

With this change, I think header file "runtime/arguments.hpp" at line 29 can be excluded.

Indeed, that header file is no longer needs to be included - thanks!

dcubed-ojdk · 2023-02-10T17:15:53Z

src/hotspot/share/runtime/continuation.hpp

-  static bool enabled(); // TODO: used while virtual threads are in Preview; remove when GA
+  static bool enabled();


Based on the comment, I was expecting this line to be removed.
I'll take it that there's still a way to disable Continuations even after virtual
threads come out of "Preview"?

Update: I see the VMContinuations option is still present and experimental.
When will it become non-experimental or removed?

And now I'm mulling why VMContinuations doesn't a default value...

Ohhh... that's right. It's not implemented on all platforms yet... got it...

That's right. Additionally, the alternative implementation of virtual threads can be tested by running tests with -XX:-VMContinuations, and we have a few tests that do this.

AlanBateman · 2023-02-11T06:19:21Z

/integrate

openjdk · 2023-02-11T06:20:52Z

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

919a6da: 8301202: Port fdlibm log to Java
98e98e9: 8296322: javac: use methods to manage parser mode flags
84e4799: 8297292: java/nio/channels/FileChannel/FileExtensionAndMap.java is too slow
57798dd: 6595142: (spec) ByteArrayInputStream treats bytes, not characters
880f52f: 8301638: A number of nsk/jdi invokemethod tests should be converted to create virtual threads
db1b48e: 8302167: Avoid allocating register in fast_lock()
582b943: 8278965: crash in SymbolTable::do_lookup
4539899: 8301480: Replace NULL with nullptr in os/posix
1428db7: 8301224: Replace NULL with nullptr in share/gc/shared/
c25b4f4: 8301578: Perform output outside synchronization in Module.class
... and 4 more: https://git.openjdk.org/jdk/compare/723433dbebc6b542dab445a89b7437149730b858...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-11T06:21:02Z

@AlanBateman Pushed as commit 74b167b.

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

Initial commit

2ef2780

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 7, 2023

openjdk bot removed the hotspot-runtime hotspot-runtime-dev@openjdk.org label Feb 8, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Feb 8, 2023

AlanBateman marked this pull request as ready for review February 8, 2023 15:12

openjdk bot added the rfr Pull request is ready for review label Feb 8, 2023

vnkozlov approved these changes Feb 8, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 8, 2023

dholmes-ora approved these changes Feb 9, 2023

View reviewed changes

AlanBateman added 2 commits February 9, 2023 13:24

Update copyright date

5adccad

Merge

38f9ccd

shqking reviewed Feb 10, 2023

View reviewed changes

AlanBateman added 2 commits February 10, 2023 07:46

Merge

0578469

Remove include

df807cb

dcubed-ojdk reviewed Feb 10, 2023

View reviewed changes

dcubed-ojdk approved these changes Feb 10, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 11, 2023

openjdk bot closed this Feb 11, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8301819: Enable continuations code by default #12459

8301819: Enable continuations code by default #12459

AlanBateman commented Feb 7, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 7, 2023

openjdk bot commented Feb 7, 2023

AlanBateman commented Feb 8, 2023

openjdk bot commented Feb 8, 2023

openjdk bot commented Feb 8, 2023

mlbridge bot commented Feb 8, 2023 •

edited

Loading

vnkozlov commented Feb 8, 2023

vnkozlov left a comment

openjdk bot commented Feb 8, 2023 •

edited

Loading

dcubed-ojdk commented Feb 8, 2023

dholmes-ora left a comment

AlanBateman commented Feb 9, 2023

shqking Feb 10, 2023

AlanBateman Feb 10, 2023

dcubed-ojdk Feb 10, 2023 •

edited

Loading

dcubed-ojdk Feb 10, 2023

dcubed-ojdk Feb 10, 2023

AlanBateman Feb 10, 2023

AlanBateman commented Feb 11, 2023

openjdk bot commented Feb 11, 2023

openjdk bot commented Feb 11, 2023

		bool Continuations::enabled() {
		return VMContinuations && Arguments::enable_preview();

		static bool enabled(); // TODO: used while virtual threads are in Preview; remove when GA
		static bool enabled();

8301819: Enable continuations code by default #12459

8301819: Enable continuations code by default #12459

Conversation

AlanBateman commented Feb 7, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 7, 2023

openjdk bot commented Feb 7, 2023

AlanBateman commented Feb 8, 2023

openjdk bot commented Feb 8, 2023

openjdk bot commented Feb 8, 2023

mlbridge bot commented Feb 8, 2023 • edited Loading

Webrevs

vnkozlov commented Feb 8, 2023

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 8, 2023 • edited Loading

dcubed-ojdk commented Feb 8, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

AlanBateman commented Feb 9, 2023

shqking Feb 10, 2023

Choose a reason for hiding this comment

AlanBateman Feb 10, 2023

Choose a reason for hiding this comment

dcubed-ojdk Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

dcubed-ojdk Feb 10, 2023

Choose a reason for hiding this comment

dcubed-ojdk Feb 10, 2023

Choose a reason for hiding this comment

AlanBateman Feb 10, 2023

Choose a reason for hiding this comment

AlanBateman commented Feb 11, 2023

openjdk bot commented Feb 11, 2023

openjdk bot commented Feb 11, 2023

AlanBateman commented Feb 7, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 8, 2023 •

edited

Loading

openjdk bot commented Feb 8, 2023 •

edited

Loading

dcubed-ojdk Feb 10, 2023 •

edited

Loading