8276658: Clean up JNI local handles code #6336

coleenp · 2021-11-10T17:16:29Z

JNI Local handles can only be created by JavaThread (there's an assert in make_local) but the fields are added to Thread.
Move the fields to JavaThread and adding JavaThread* argument.
Also, the global freelist isn't very useful now that global JNI handles don't use JNIHandleBlock, so the locking that claims incorrectly to block for safepoint is removed.
Lastly, there's at least 3 places that duplicate pushing a new JNIHandleBlock to the thread for temporarily adding JNI local handles. These have been moved to common code with a JNIHandleMark object, moved from jvmci code.
The commits are separate to help reviewing, but the entire change has been tested together with tier1-6.
The commits in this change have been performance tested individually and together with no meaningful differences from mainline.

Progress

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

Issue

JDK-8276658: Clean up JNI local handles code

Reviewers

David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to f31dfee
Patricio Chilano Mateo (@pchilano - Committer) ⚠️ Review applies to f31dfee

Reviewing

Using git

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

Update a local copy of the PR:
$ git checkout pull/6336
$ git pull https://git.openjdk.java.net/jdk pull/6336/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6336

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6336.diff

…t then it allocates a block on its own thread, which it never cleans up. Pass the caller thread to allocate_handle so that allocate_block will add to the right thread, which is a JavaThread.

bridgekeeper · 2021-11-10T17:17:39Z

👋 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 · 2021-11-10T17:19:29Z

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

hotspot
serviceability

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 · 2021-11-10T17:24:46Z

Webrevs

dholmes-ora

Hi Coleen,

Nice cleanup and refactoring! I'm not familiar with all the details but the reshuffling looks good to me.

One query and one minor issue below.

Thanks,
David

dholmes-ora · 2021-11-11T06:35:59Z

src/hotspot/share/runtime/vmThread.hpp

@@ -59,6 +60,8 @@ class VMOperationTimeoutTask : public PeriodicTask {

 class VMThread: public NamedThread {
 private:
+  volatile bool _is_running;


I don't see this being initialized to false.

Good catch!

src/hotspot/share/compiler/compileBroker.cpp

openjdk · 2021-11-11T07:09:09Z

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

8276658: Clean up JNI local handles code

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

aeba653: 8276743: Make openjdk build Zip Archive generation "reproducible"
51a5731: 8277016: Use blessed modifier order in jdk.httpserver
c4b4432: 8277012: Use blessed modifier order in src/utils
13deb38: 8276848: sun.net.httpserver.simpleserver.CommandLinePositiveTest: test does not specify port
710f496: 8273277: C2: Move conditional negation into rc_predicate
6b833db: 8275329: ZGC: vmTestbase/gc/gctests/SoftReference/soft004/soft004.java fails with assert(_phases->length() <= 1000) failed: Too many recored phases?
1e941de: 8275197: Remove unused fields in ThaiBuddhistChronology
6954b98: 8186670: Implement _onSpinWait() intrinsic for AArch64
3445e50: 8276265: jcmd man page is outdated
0ca0acf: 8276947: Clarify how DateTimeFormatterBuilder.appendFraction handles value ranges
... and 49 more: https://git.openjdk.java.net/jdk/compare/945f4085e5c51f37c2048bb221a1521895ba28c6...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.

coleenp · 2021-11-11T13:54:14Z

Thank you for the code review, David.

pchilano

Hi Coleen,

Cleanup looks good to me.

Thanks,
Patricio

pchilano · 2021-11-12T15:08:24Z

src/hotspot/share/jfr/dcmd/jfrDcmds.cpp

-  JNIHandleBlockManager jni_handle_management(THREAD);
+  JNIHandleMark jni_handle_management(THREAD);
+
+  DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(THREAD));


This method will call into Java below which already checks the thread is in vm so maybe this is not necessary. Even construct_dcmd_instance() has that assert.

You're right, it's doubly redundant. I'll remove it.

coleenp · 2021-11-12T16:15:42Z

Thanks for the review, Patricio and David.
/integrate

openjdk · 2021-11-12T16:17:31Z

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

aeba653: 8276743: Make openjdk build Zip Archive generation "reproducible"
51a5731: 8277016: Use blessed modifier order in jdk.httpserver
c4b4432: 8277012: Use blessed modifier order in src/utils
13deb38: 8276848: sun.net.httpserver.simpleserver.CommandLinePositiveTest: test does not specify port
710f496: 8273277: C2: Move conditional negation into rc_predicate
6b833db: 8275329: ZGC: vmTestbase/gc/gctests/SoftReference/soft004/soft004.java fails with assert(_phases->length() <= 1000) failed: Too many recored phases?
1e941de: 8275197: Remove unused fields in ThaiBuddhistChronology
6954b98: 8186670: Implement _onSpinWait() intrinsic for AArch64
3445e50: 8276265: jcmd man page is outdated
0ca0acf: 8276947: Clarify how DateTimeFormatterBuilder.appendFraction handles value ranges
... and 49 more: https://git.openjdk.java.net/jdk/compare/945f4085e5c51f37c2048bb221a1521895ba28c6...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-12T16:17:47Z

@coleenp Pushed as commit 3b2585c.

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

coleenp added 4 commits November 9, 2021 12:56

Move active_handles to JavaThread.

8be58c0

Remove JNIHandleBlock global freelists and Mutex

2a38de8

Refactor pushing and popping JNIHandleBlocks.

7229b97

The VM Thread creates handles on the caller thread, unless it runs ou…

239e924

…t then it allocates a block on its own thread, which it never cleans up. Pass the caller thread to allocate_handle so that allocate_block will add to the right thread, which is a JavaThread.

openjdk bot added the rfr Pull request is ready for review label Nov 10, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Nov 10, 2021

dholmes-ora approved these changes Nov 11, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 11, 2021

Add _is_running initialization.

f31dfee

dholmes-ora approved these changes Nov 12, 2021

View reviewed changes

pchilano approved these changes Nov 12, 2021

View reviewed changes

Remove redundant assert.

f24e32c

openjdk bot closed this Nov 12, 2021

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 Nov 12, 2021

coleenp deleted the jni branch November 12, 2021 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8276658: Clean up JNI local handles code #6336

8276658: Clean up JNI local handles code #6336

coleenp commented Nov 10, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 10, 2021

openjdk bot commented Nov 10, 2021

mlbridge bot commented Nov 10, 2021 •

edited

Loading

dholmes-ora left a comment •

edited

Loading

dholmes-ora Nov 11, 2021

coleenp Nov 11, 2021

openjdk bot commented Nov 11, 2021 •

edited

Loading

coleenp commented Nov 11, 2021

pchilano left a comment

pchilano Nov 12, 2021

coleenp Nov 12, 2021

coleenp commented Nov 12, 2021

openjdk bot commented Nov 12, 2021

openjdk bot commented Nov 12, 2021

8276658: Clean up JNI local handles code #6336

8276658: Clean up JNI local handles code #6336

Conversation

coleenp commented Nov 10, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 10, 2021

openjdk bot commented Nov 10, 2021

mlbridge bot commented Nov 10, 2021 • edited Loading

Webrevs

dholmes-ora left a comment • edited Loading

Choose a reason for hiding this comment

dholmes-ora Nov 11, 2021

Choose a reason for hiding this comment

coleenp Nov 11, 2021

Choose a reason for hiding this comment

openjdk bot commented Nov 11, 2021 • edited Loading

coleenp commented Nov 11, 2021

pchilano left a comment

Choose a reason for hiding this comment

pchilano Nov 12, 2021

Choose a reason for hiding this comment

coleenp Nov 12, 2021

Choose a reason for hiding this comment

coleenp commented Nov 12, 2021

openjdk bot commented Nov 12, 2021

openjdk bot commented Nov 12, 2021

coleenp commented Nov 10, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 10, 2021 •

edited

Loading

dholmes-ora left a comment •

edited

Loading

openjdk bot commented Nov 11, 2021 •

edited

Loading