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

8306647: Implementation of Structured Concurrency (Preview) #13932

Closed
wants to merge 18 commits into from

Conversation

AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented May 11, 2023

This is the implementation of:

  • JEP 453: Structured Concurrency (Preview)
  • JEP 446: Scoped Values (Preview)

For the most part, this is just moving code and tests. StructuredTaskScope moves to j.u.concurrent as a preview API, ScopedValue moves to j.lang as a preview API, and module jdk.incubator.concurrent has been removed. The significant API changes since incubator are:

  • StructuredTaskScope.fork returns Subtask instead of Future (JEP 453 has a section on this)
  • ScopedValue.where methods are replaced with runWhere, callWhere and getWhere

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Change requires CSR request JDK-8306573 to be approved
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8306916 to be approved

Issues

  • JDK-8306647: Implementation of Structured Concurrency (Preview) (Enhancement - "3")
  • JDK-8306572: Implementation of Scoped Values (Preview) (Enhancement - "4")
  • JDK-8306916: Implementation of Structured Concurrency (Preview) (CSR)
  • JDK-8306573: Implementation of Scoped Values (Preview) (CSR)

Reviewers

Contributors

  • Alan Bateman <alanb@openjdk.org>
  • Andrew Haley <aph@openjdk.org>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13932

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented May 11, 2023

👋 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 openjdk bot added the csr Pull request needs approved CSR before integration label May 11, 2023
@openjdk
Copy link

openjdk bot commented May 11, 2023

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

  • build
  • core-libs
  • hotspot
  • security

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 security security-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels May 11, 2023
@AlanBateman
Copy link
Contributor Author

/issue add JDK-8306647
/issue add JDK-8306572
/contributor add alanb
/contributor add aph
/label remove build
/label remove security

@openjdk
Copy link

openjdk bot commented May 11, 2023

@AlanBateman This issue is referenced in the PR title - it will now be updated.

@openjdk
Copy link

openjdk bot commented May 11, 2023

@AlanBateman
Adding additional issue to issue list: 8306572: Implementation of Scoped Values (Preview).

@openjdk
Copy link

openjdk bot commented May 11, 2023

@AlanBateman
Contributor Alan Bateman <alanb@openjdk.org> successfully added.

@openjdk
Copy link

openjdk bot commented May 11, 2023

@AlanBateman
Contributor Andrew Haley <aph@openjdk.org> successfully added.

@openjdk openjdk bot removed the build build-dev@openjdk.org label May 11, 2023
@openjdk
Copy link

openjdk bot commented May 11, 2023

@AlanBateman
The build label was successfully removed.

@openjdk openjdk bot removed the security security-dev@openjdk.org label May 11, 2023
@openjdk
Copy link

openjdk bot commented May 11, 2023

@AlanBateman
The security label was successfully removed.

Cache.invalidate(bitmask);
var prevSnapshot = scopedValueBindings();
var newSnapshot = new Snapshot(this, prevSnapshot);
return runWith(newSnapshot, new CallableAdapter<R>(op));
Copy link
Member

Choose a reason for hiding this comment

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

Can we just do this instead?

Suggested change
return runWith(newSnapshot, new CallableAdapter<R>(op));
return runWith(newSnapshot, op::get);

IIUC the current approach is to avoid the dynamic creation of a class via the invoke dynamic? I don't fully understand the comment about release fencing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@theRealAph will want to comment on this as it is very performance sensitive. I think CallableAdapter.s is non-final to avoid the release fence.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right. The problem is that we can never get rid of the release fence, apparently even when the instance of the adapter is scalar replaced. I imagine that'll get fixed one day, but this is internal JDK code.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and i guess that has some performance implications in some cases? more so on the set of instructions produced on ARM say rather than limiting C2 optimizations?

Given that the Supplier is likely to be the target of a lambda expression which may also capture I was surprised there would be much of an increased impact. (HotSpot may not strength reduce multiple fences in this case.)

Can we update to add say "/* non-final */ to the field and update the class doc to say the release fence inserted by HotSpot as a result of constructing a class with final fields has performance implications <<"insert loose quantification of those implications">> ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, and i guess that has some performance implications in some cases? more so on the set of instructions produced on ARM say rather than limiting C2 optimizations?

I think so. As far as I can remember, a release fence on x86 generates no code at all.

Given that the Supplier is likely to be the target of a lambda expression which may also capture I was surprised there would be much of an increased impact. (HotSpot may not strength reduce multiple fences in this case.)

It may, or it may not. I don't really want the call without a checked exception to be more costly than a call with one. That seems a little weird, at least.

Can we update to add say "/* non-final */ to the field and update the class doc to say the release fence inserted by HotSpot as a result of constructing a class with final fields has performance implications <<"insert loose quantification of those implications">> ?

Sure, thanks.

@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label May 25, 2023
Copy link
Member

@PaulSandoz PaulSandoz left a comment

Choose a reason for hiding this comment

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

This looks good (both ScopedValue and StructuredTaskScope).

I recall some discussion on the loom email list noting that ScopedValue instances are like capabilities and should be shared carefully (like MethodHandles in some sense). I think it would be useful to follow up on that in the specification.

I am uncertain on the name Subtask but cannot think of anything better, perhaps during this round of preview we might find something else.

For a small API StructureTaskScope is rather powerful and deftly combines language, library, and platform features (virtual threads).

I suggest revisiting the constraints on operating on SubTask instances. I get the motivation but i wonder if the additional specification and implementation work required to properly describe the behaviour and enforce it is really worth it. Such a restriction tends to feel artificial given there are no side-effects to such operations, and further we cannot enforce such restrictions in the compiler.
The constraints of fork, join, shutdown, and close are key with the idiomatic use with try-with-resources. Just let subtasks be accessible by any thread for all its methods and provide clear documentation on the recommended use.

@forax
Copy link
Member

forax commented May 30, 2023

I've rewritten all my tests to use the new API, it works !
https://github.com/forax/loom-fiber/tree/java21

One surprising thing is that Subtask.get() give less leeway to the owner thread than to the other virtual threads so in onComplete() storing a Subtask to use it later by the owner thread does not work well if join() is not called yet.
This is perhaps a bit too much.

Anyway, good for me !

@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label May 31, 2023
@AlanBateman
Copy link
Contributor Author

One surprising thing is that Subtask.get() give less leeway to the owner thread than to the other virtual threads so in onComplete() storing a Subtask to use it later by the owner thread does not work well if join() is not called yet. This is perhaps a bit too much.

Maybe but it would be a bug if the owner were to call Subtask::get before joining. This is easy to detect for subtasks that are forked by the owner. Time will tell how common it will be for subtasks to fork additional subtasks.

Copy link
Member

@dfuch dfuch left a comment

Choose a reason for hiding this comment

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

I reviewed the API documentation of ScopedValue (mostly the class level API documentation, but I had a look at the methods API too) and it looks good. It reads well, and when I had question they were usually answered in the next paragraph that followed what I was reading.

I didn't look at Structured Concurrency.

@openjdk
Copy link

openjdk bot commented Jun 3, 2023

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

8306647: Implementation of Structured Concurrency (Preview)
8306572: Implementation of Scoped Values (Preview)

Co-authored-by: Alan Bateman <alanb@openjdk.org>
Co-authored-by: Andrew Haley <aph@openjdk.org>
Reviewed-by: psandoz, dfuchs, mchung

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

  • 9526190: 8307840: SequencedMap view method specification and implementation adjustments
  • 74dc50b: 8301721: lookup.findSpecial fails on Object method call from interface
  • 0f0fda7: 8309542: compiler/jvmci/TestEnableJVMCIProduct.java fails with "JVMCI compiler 'graal' specified by jvmci.Compiler not found"
  • 38cef2a: 8309413: Improve the performance of MethodTypeDesc::descriptorString
  • 7edd054: 8309501: Remove workaround in bin/idea.sh for non standard JVMCI file layout
  • 9188142: 8309216: Cast from jchar* to char* in test java/io/GetXSpace.java
  • d709c25: 8307887: (fs) Files.createSymbolicLink throws less specific exception when in developer mode and file already exists
  • ca6f07f: 8309534: @jep(number=430, title="String Templates") should use default status
  • 8f0839b: 8308748: JNU_GetStringPlatformChars may write to String's internal memory array
  • 01455a0: 8304878: ConcurrentModificationException in javadoc tool
  • ... and 5 more: https://git.openjdk.org/jdk/compare/2e9eff56418273e85accc43dcef533995c6be8bf...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 Jun 3, 2023
Copy link
Member

@mlchung mlchung left a comment

Choose a reason for hiding this comment

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

I reviewed the implementation changes to promote an incubating API to a preview API. That part looks good.

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jun 5, 2023
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jun 6, 2023
@AlanBateman
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jun 7, 2023

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

  • a08c5cb: 8307953: [AIX] C locale's font setting was changed by JEP 400
  • 0ceb432: 8309570: ProblemList sun/security/pkcs11/Signature/TestRSAKeyLength.java
  • 65bdbc7: 8309396: com/sun/jdi/JdbMethodExitTest.java fails with virtual threads due to a bug in determining the main thread id
  • 4a75fd4: 8301553: Support Password-Based Cryptography in SunPKCS11
  • 0a4f9ad: 8292157: Incorrect error: "block element not allowed within inline element "
  • 16ab7bf: 8309505: com/sun/jdi/MethodEntryExitEvents.java due to finding wrong main thread
  • d82436e: 8295071: Spec Clarification : ClassFileFormatVersion: System property java.class.version | Java class format version number
  • 571fbdc: 8309506: com/sun/jdi/MultiBreakpointsTest.java fails with virtual test thread factory
  • 7d1147e: 8309554: Update descriptions in SourceVersion
  • 9526190: 8307840: SequencedMap view method specification and implementation adjustments
  • ... and 14 more: https://git.openjdk.org/jdk/compare/2e9eff56418273e85accc43dcef533995c6be8bf...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 7, 2023
@openjdk openjdk bot closed this Jun 7, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 7, 2023
@openjdk
Copy link

openjdk bot commented Jun 7, 2023

@AlanBateman Pushed as commit f1c7afc.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
7 participants