8310644: Make panama memory segment close use async handshakes #16792

fisk · 2023-11-23T11:14:29Z

The current logic for closing memory in panama today is susceptible to live lock if we have a closing thread that wants to close the memory in a loop that keeps failing, and a bunch of accessing threads that want to perform accesses as long as the memory is alive. They can both create impediments for the other.

By using asynchronous handshakes to install an exception onto threads that are in @Scoped memory accesses, we can have close always succeed, and the accessing threads bail out. The idea is that we perform a synchronous handshake first to find threads that are in scoped methods. They might however be in the middle of throwing an exception or something wild like there, where an exception can't be delivered. We install an async handshake that will roll us forward to the first place where we can indeed install exceptions, then we reevaluate if we still need to do that, or if we have unwound out from the scoped method. If we are still inside of it, we ensure an exception is installed so we don't continue executing bytecodes that might access the memory that we have freed.

Tested tier 1-5 as well as running test/jdk/java/foreign/TestHandshake.java hundreds of times, which tests this API pretty well.

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-8310644: Make panama memory segment close use async handshakes (Enhancement - P3)

Reviewers

Jorn Vernee (@JornVernee - Reviewer) ⚠️ Review applies to 5d34ddba
Maurizio Cimadamore (@mcimadamore - Reviewer)
Patricio Chilano Mateo (@pchilano - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16792

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-11-23T11:15:28Z

👋 Welcome back eosterlund! 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-11-23T11:17:50Z

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

core-libs
hotspot

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.

mlbridge · 2023-11-23T11:21:14Z

Webrevs

JornVernee · 2023-11-23T11:31:59Z

src/java.base/share/classes/jdk/internal/foreign/SharedSession.java

-        boolean success = SCOPED_MEMORY_ACCESS.closeScope(this);
-        STATE.setVolatile(this, success ? CLOSED : OPEN);
-        if (!success) {
-            throw alreadyAcquired(1);
-        }
+        SCOPED_MEMORY_ACCESS.closeScope(this);


JornVernee · 2023-11-23T11:34:14Z

src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template

-    public boolean closeScope(MemorySessionImpl session) {
-        return closeScope0(session);
+    public void closeScope(MemorySessionImpl session) {
+        closeScope0(session, MemorySessionImpl.ALREADY_CLOSED);


I suggest passing in the ALREADY_CLOSED instance as an argument to this method instead. Then we can avoid making the field in MemorySessionImpl public.

JornVernee · 2023-11-23T11:43:38Z

src/hotspot/share/prims/scopedMemoryAccess.cpp

-    if (_deopt != nullptr && last_frame.is_compiled_frame() && last_frame.can_be_deoptimized()) {
-      CloseScopedMemoryFindOopClosure cl(_deopt);
+    if (_session != nullptr && last_frame.is_compiled_frame() && last_frame.can_be_deoptimized()) {
+      CloseScopedMemoryFindOopClosure cl(_session);


Pre-existing, but this value (and class) is unused since we do an unconditional deopt. If you feel like it, you could remove the CloseScopedMemoryFindOopClosure. We can get it back from the git history later when that bug is fixed (https://bugs.openjdk.org/browse/JDK-8290892)

test/jdk/java/foreign/TestHandshake.java

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>

fisk · 2023-11-23T16:30:53Z

Thanks for the review @JornVernee! I applied the changes you wanted I think.

JornVernee

LGTM

openjdk · 2023-11-23T16:34:19Z

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

8310644: Make panama memory segment close use async handshakes

Reviewed-by: jvernee, mcimadamore, pchilanomate

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

86bb804: 8320863: dsymutil command leaves around temporary directories
db7fedf: 8320358: GHA: ignore jdk* branches
e33b6c1: 8319437: NMT should show library names in call stacks
2fae07f: 8319311: JShell Process Builder should be configurable
63ad868: 8319668: Fixup of jar filename typo in BadFactoryTest.sh
4bcda60: 8319713: Parallel: Remove PSAdaptiveSizePolicy::should_full_GC
99f870c: 8320781: Fix whitespace in j.l.Double and j.u.z.ZipInputStream @snippets
a5ccd3b: 8267532: C2: Profile and prune untaken exception handlers
464dc3d: 8319633: runtime/posixSig/TestPosixSig.java intermittent timeouts on UNIX
efc3922: 8319048: Monitor deflation unlink phase prolongs time to safepoint
... and 37 more: https://git.openjdk.org/jdk/compare/99b9cb0a2eae1196f2127cd48e4085f19a90c4b4...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.

…CHECK

openjdk · 2023-11-24T14:17:05Z

⚠️ @fisk This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

mcimadamore

The new logic looks good. Good catch on the array copy with swap!

pchilano

I don't know about the Panama details but the asynchronous handshake usage looks good.

fisk · 2023-11-29T12:37:26Z

Thanks for the reviews @JornVernee @pchilano and @mcimadamore!

fisk · 2023-11-29T12:38:44Z

/integrate

openjdk · 2023-11-29T12:40:26Z

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

65dfcae: 8308399: Recommend --release when -source and -target are misused
335f5db: 8320911: RISC-V: Enable hotspot/jtreg/compiler/intrinsics/chacha/TestChaCha20.java
77d604a: 8319373: Serial: Refactor dirty cards scanning during Young GC
38cfb22: 8318706: Implement JEP 423: Region Pinning for G1
e44d4b2: 8320858: Move jpackage tests to tier3
5dcf3a5: 8320715: Improve the tests of test/hotspot/jtreg/compiler/intrinsics/float16
78b6c2b: 8320898: exclude compiler/vectorapi/reshape/TestVectorReinterpret.java on ppc64(le) platforms
9a6ca23: 8320918: Fix errors in the built-in Catalog implementation
5e1b771: 8316422: TestIntegerUnsignedDivMod.java triggers "invalid layout" assert in FrameValues::validate
a657aa3: 8320681: [macos] Test tools/jpackage/macosx/MacAppStoreJlinkOptionsTest.java timed out on macOS
... and 57 more: https://git.openjdk.org/jdk/compare/99b9cb0a2eae1196f2127cd48e4085f19a90c4b4...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-29T12:40:32Z

@fisk Pushed as commit 1594653.

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

uschindler · 2023-12-01T17:59:13Z

I can confirm: This works fine with Lucene! The isAlive state is visible in other threads and closing the Arena's scope can no longer throw IllegalStateException. See comment here: apache/lucene#12706 (comment)

8310644: Make panama memory segment close use async handshakes

aac442a

openjdk bot added the rfr Pull request is ready for review label Nov 23, 2023

openjdk bot added hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Nov 23, 2023

JornVernee reviewed Nov 23, 2023

View reviewed changes

fisk and others added 2 commits November 23, 2023 17:20

Update test/jdk/java/foreign/TestHandshake.java

d12fa90

Co-authored-by: Jorn Vernee <JornVernee@users.noreply.github.com>

Comments from Jorn

5d34ddb

JornVernee approved these changes Nov 23, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 23, 2023

JornVernee and others added 5 commits November 24, 2023 12:18

use more fine grained exception check

5e551d9

work around NoSafepointVerifier check in CopySwapMemory

f7e3f51

Polish: use special UNSAFE_ENTRY_SCOPED

9107d70

Add swappy copy cast to TestHandShake.java

057a176

Merge pull request #2 from JornVernee/PR_async_close+ASYNC_EXCEPTION_…

0b91ac9

…CHECK

JornVernee and others added 2 commits November 24, 2023 16:25

don't transition to native state on Unsafe_CopySwapMemory0

f787771

Merge pull request #3 from JornVernee/PR_async_close+NoToNativeTrans

83cef37

mcimadamore approved these changes Nov 27, 2023

View reviewed changes

pchilano approved these changes Nov 28, 2023

View reviewed changes

JornVernee mentioned this pull request Nov 28, 2023

Draft: Improve Unsafe::setMemory for small writes #16760

Closed

3 tasks

openjdk bot added the integrated Pull request has been integrated label Nov 29, 2023

openjdk bot closed this Nov 29, 2023

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

uschindler mentioned this pull request Nov 30, 2023

Enable MemorySegment in MMapDirectory for Java 22+ and Vectorization (incubation) for exact Java 22 apache/lucene#12706

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8310644: Make panama memory segment close use async handshakes #16792

8310644: Make panama memory segment close use async handshakes #16792

fisk commented Nov 23, 2023 •

edited by openjdk bot

bridgekeeper bot commented Nov 23, 2023

openjdk bot commented Nov 23, 2023

mlbridge bot commented Nov 23, 2023 •

edited

JornVernee Nov 23, 2023

JornVernee Nov 23, 2023

JornVernee Nov 23, 2023

fisk commented Nov 23, 2023

JornVernee left a comment

openjdk bot commented Nov 23, 2023 •

edited

openjdk bot commented Nov 24, 2023

mcimadamore left a comment

pchilano left a comment

fisk commented Nov 29, 2023

fisk commented Nov 29, 2023

openjdk bot commented Nov 29, 2023

openjdk bot commented Nov 29, 2023

uschindler commented Dec 1, 2023

8310644: Make panama memory segment close use async handshakes #16792

8310644: Make panama memory segment close use async handshakes #16792

Conversation

fisk commented Nov 23, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Nov 23, 2023

openjdk bot commented Nov 23, 2023

mlbridge bot commented Nov 23, 2023 • edited

Webrevs

JornVernee Nov 23, 2023

Choose a reason for hiding this comment

JornVernee Nov 23, 2023

Choose a reason for hiding this comment

JornVernee Nov 23, 2023

Choose a reason for hiding this comment

fisk commented Nov 23, 2023

JornVernee left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 23, 2023 • edited

openjdk bot commented Nov 24, 2023

mcimadamore left a comment

Choose a reason for hiding this comment

pchilano left a comment

Choose a reason for hiding this comment

fisk commented Nov 29, 2023

fisk commented Nov 29, 2023

openjdk bot commented Nov 29, 2023

openjdk bot commented Nov 29, 2023

uschindler commented Dec 1, 2023

fisk commented Nov 23, 2023 •

edited by openjdk bot

mlbridge bot commented Nov 23, 2023 •

edited

openjdk bot commented Nov 23, 2023 •

edited