8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock by coleenp · Pull Request #17739 · openjdk/jdk

coleenp · 2024-02-06T22:59:04Z

This change creates a new sort of native recursive lock that can be held during JNI and Java calls, which can be used for synchronization while creating objArrayKlasses at runtime.

Passes tier1-7.

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-8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock (Bug - P4)

Reviewers

Dean Long (@dean-long - Reviewer) ⚠️ Review applies to 3625e6cc
David Holmes (@dholmes-ora - Reviewer)
Frederic Parain (@fparain - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17739

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-06T22:59:53Z

👋 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-02-06T23:00:39Z

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

src/hotspot/share/runtime/recursiveLock.cpp

src/hotspot/share/runtime/recursiveLock.hpp

src/hotspot/share/classfile/classLoaderData.cpp

src/hotspot/share/oops/arrayKlass.cpp

src/hotspot/share/prims/jvmtiExport.cpp

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp

coleenp

Thanks for looking at this WIP.

src/hotspot/share/oops/arrayKlass.cpp

src/hotspot/share/prims/jvmtiExport.cpp

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp

src/hotspot/share/runtime/recursiveLock.cpp

…ile holding a lock

mlbridge · 2024-03-05T19:50:09Z

Webrevs

dean-long · 2024-03-05T23:13:30Z

Is the caller always a JavaThread? I'm wondering if your new recursive lock class could use the existing ObjectMonitor. I thought David asked the same question, but I can't find it.

dholmes-ora · 2024-03-06T06:58:13Z

I'm wondering if your new recursive lock class could use the existing ObjectMonitor.

There has been a drive to remove ObjectLocker from the C++ code due to the negative impact on Loom. Also not sure what existing ObjectMonitor you are referring to. ??

coleenp · 2024-03-06T12:57:09Z

@dean-long An ObjectLocker on the mirror oop for InstanceKlass might work for this but as @dholmes-ora said we've been trying to purge the ObjectLocker code from the C++ code because it's complicated by the Java Object monitor project. The JOM project may throw exceptions from the ObjectLocker constructor or destructor and the C++ code doesn't currently know what to do. We removed the ObjectLockers around class linking and some JVMTI cases. There are some in class loading but with a small amount of work, they can be removed also.

The caller is always a JavaThread, some lockers are at a safepoint for traversal but the multi-arrays are only created by JavaThreads.

dean-long · 2024-03-06T23:07:47Z

OK, that makes sense about loom and JOM.

dean-long · 2024-03-06T23:09:34Z

src/hotspot/share/runtime/mutex.cpp

+  } else {
+    // can be called by jvmti by VMThread.
+    if (current->is_Java_thread()) {
+      _sem.wait_with_safepoint_check(JavaThread::cast(current));


Why not use PlatformMutex + OSThreadWaitState instead of a semaphore?

Semaphore seemed simpler (?)

OK. It's a good thing HotSpot doesn't need to worry about priority-inheritance for mutexes.

Semaphore seems simpler/cleaner and ready to use.

It's such a rare race and unusual condition that we could execute more Java code, that I started out with just a shared bit. Then David suggested a semaphore that obeys the safepoint protocol, which seems a lot better. I've literally skimmed over OSThreadWaitState. It looks like Semaphore::wait_with_a_safepoint_check() uses it. I still don't know why it exists:

// Note: the ThreadState is legacy code and is not correctly implemented. // Uses of ThreadState need to be replaced by the state in the JavaThread. enum ThreadState {

Does a PlatformMutex handle priority-inheritance? It still feels like it would be overkill for this usage. I hope this RecursiveLocker does not become more widely used, where we would have to replace the simple implementation with something more difficult. We should discourage further use when possible.

Thanks for your last comment because I was worried there's a lot of other code I should know about.

src/hotspot/share/oops/arrayKlass.cpp

src/hotspot/share/oops/instanceKlass.cpp

openjdk · 2024-03-07T00:38:36Z

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

8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock

Reviewed-by: dlong, dholmes, fparain

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

de428da: 8327426: RISC-V: Move alignment shim into initialize_header() in C1_MacroAssembler::allocate_array
8dbd4b3: 8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate
7c5e6e7: 8327147: Improve performance of Math ceil, floor, and rint for x86
972e81d: 8326611: Clean up vmTestbase/nsk/stress/stack tests
5aae803: 8327390: JitTester: Implement temporary folder functionality
784f11c: 8327238: Remove MetadataAllocationFailALot* develop flags
d7273ac: 8320646: RISC-V: C2 VectorCastHF2F
53c4714: 8327501: Common ForkJoinPool prevents class unloading in some cases
1261740: 8327283: RISC-V: Minimal build failed after JDK-8319716
f54e598: 8327172: C2 SuperWord: data node in loop has no input in loop: replace assert with bailout
... and 81 more: https://git.openjdk.org/jdk/compare/8f6edd8dc866bf970b7e7b8358f62832887e6e8b...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

Thanks for your review Dean. I've retested tier1 with the changes and will send it for more. Thank you for finding the CDS bug in my change.

src/hotspot/share/oops/arrayKlass.cpp

src/hotspot/share/oops/instanceKlass.cpp

coleenp · 2024-03-07T01:31:09Z

src/hotspot/share/runtime/mutex.cpp

+  } else {
+    // can be called by jvmti by VMThread.
+    if (current->is_Java_thread()) {
+      _sem.wait_with_safepoint_check(JavaThread::cast(current));


It's such a rare race and unusual condition that we could execute more Java code, that I started out with just a shared bit. Then David suggested a semaphore that obeys the safepoint protocol, which seems a lot better. I've literally skimmed over OSThreadWaitState. It looks like Semaphore::wait_with_a_safepoint_check() uses it. I still don't know why it exists:

// Note: the ThreadState is legacy code and is not correctly implemented. // Uses of ThreadState need to be replaced by the state in the JavaThread. enum ThreadState {

Does a PlatformMutex handle priority-inheritance? It still feels like it would be overkill for this usage. I hope this RecursiveLocker does not become more widely used, where we would have to replace the simple implementation with something more difficult. We should discourage further use when possible.

src/hotspot/share/oops/instanceKlass.cpp

dean-long · 2024-03-07T02:08:26Z

Does a PlatformMutex handle priority-inheritance?

It would depend on the OS and the mutex impementation. You can ignore my comment. It was from long ago trying to put Java on top of a real-time OS.

dholmes-ora

This looks good to me. Thanks for working through the details with me.

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp

src/hotspot/share/runtime/mutexLocker.hpp

coleenp

David, thank you for your discussion while fixing this problem.

src/hotspot/share/oops/instanceKlass.cpp

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp

coleenp · 2024-03-07T13:14:06Z

src/hotspot/share/runtime/mutex.cpp

+  } else {
+    // can be called by jvmti by VMThread.
+    if (current->is_Java_thread()) {
+      _sem.wait_with_safepoint_check(JavaThread::cast(current));


Thanks for your last comment because I was worried there's a lot of other code I should know about.

src/hotspot/share/runtime/mutexLocker.hpp

fparain

LGTM

dholmes-ora · 2024-03-08T06:17:06Z

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp

  {
    // To get a consistent list of classes we need MultiArray_lock to ensure
-    // array classes aren't created during this walk. This walks through the
+    // array classes aren't created by another thread during this walk. This walks through the


Thanks for clarifying, though I'm not sure why would care given the set of classes could have changed by the time we return them anyway.

I think in this case, we might find an ObjArrayKlass without the mirror since the Klass is added to the higher_dimension links before the mirror is created. The lock keeps them both together.

coleenp · 2024-03-08T13:27:15Z

Thank you for reviewing, Dean, David and Fred.
/integrate

openjdk · 2024-03-08T13:27:55Z

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

997e615: 8322943: runtime/CompressedOops/CompressedClassPointers.java fails on AIX
27a03e0: 8327218: Add an ability to specify modules which should have native access enabled
d0d4912: 8326096: Deprecate getTotalIn, getTotalOut methods of java.util.zip.Inflater, java.util.zip.Deflater
c65da92: 8327571: Parallel: Remove redundant operation in PSParallelCompact::clear_data_covering_space
f9d479f: 8327477: Parallel: Remove _data_location and _highest_ref in ParallelCompactData
bdd1aeb: 8312383: Log X509ExtendedKeyManager implementation class name in TLS/SSL connection
fb4610e: 8327444: simplify RESTARTABLE macro usage in JDK codebase
de428da: 8327426: RISC-V: Move alignment shim into initialize_header() in C1_MacroAssembler::allocate_array
8dbd4b3: 8326446: The User and System of jdk.CPULoad on Apple M1 are inaccurate
7c5e6e7: 8327147: Improve performance of Math ceil, floor, and rint for x86
... and 88 more: https://git.openjdk.org/jdk/compare/8f6edd8dc866bf970b7e7b8358f62832887e6e8b...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-03-08T13:28:02Z

@coleenp Pushed as commit 1877a48.

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

coleenp force-pushed the objArray2 branch from 570d776 to 1c7b010 Compare February 6, 2024 22:59

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Feb 6, 2024

dholmes-ora reviewed Feb 8, 2024

View reviewed changes

coleenp commented Feb 13, 2024

View reviewed changes

coleenp force-pushed the objArray2 branch from 1c7b010 to a9ce2e5 Compare February 13, 2024 02:55

coleenp force-pushed the objArray2 branch from a9ce2e5 to d682f54 Compare February 29, 2024 16:40

8308745: ObjArrayKlass::allocate_objArray_klass may call into java wh…

3625e6c

…ile holding a lock

coleenp force-pushed the objArray2 branch from d682f54 to 3625e6c Compare February 29, 2024 16:43

coleenp marked this pull request as ready for review March 5, 2024 19:46

openjdk bot added the rfr Pull request is ready for review label Mar 5, 2024

dean-long reviewed Mar 6, 2024

View reviewed changes

dean-long reviewed Mar 7, 2024

View reviewed changes

src/hotspot/share/oops/arrayKlass.cpp Show resolved Hide resolved

dean-long reviewed Mar 7, 2024

View reviewed changes

src/hotspot/share/oops/arrayKlass.cpp Show resolved Hide resolved

dean-long reviewed Mar 7, 2024

View reviewed changes

src/hotspot/share/oops/instanceKlass.cpp Show resolved Hide resolved

dean-long approved these changes Mar 7, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 7, 2024

Dean's comments.

d64aa56

coleenp commented Mar 7, 2024

View reviewed changes

dholmes-ora reviewed Mar 7, 2024

View reviewed changes

src/hotspot/share/oops/instanceKlass.cpp Outdated Show resolved Hide resolved

dholmes-ora approved these changes Mar 7, 2024

View reviewed changes

src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp Outdated Show resolved Hide resolved

src/hotspot/share/runtime/mutexLocker.hpp Outdated Show resolved Hide resolved

coleenp commented Mar 7, 2024

View reviewed changes

coleenp added 2 commits March 7, 2024 13:16

David's comment fixes.

f643760

Missed a word.

88fd6f1

fparain approved these changes Mar 7, 2024

View reviewed changes

dholmes-ora approved these changes Mar 8, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 8, 2024

openjdk bot closed this Mar 8, 2024

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

coleenp deleted the objArray2 branch March 8, 2024 13:28

Conversation

coleenp commented Feb 6, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Feb 6, 2024

Uh oh!

openjdk bot commented Feb 6, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mlbridge bot commented Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

dean-long commented Mar 5, 2024

Uh oh!

dholmes-ora commented Mar 6, 2024

Uh oh!

coleenp commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dean-long commented Mar 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openjdk bot commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dean-long commented Mar 7, 2024

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Feb 6, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 5, 2024 •

edited

Loading

coleenp commented Mar 6, 2024 •

edited

Loading

openjdk bot commented Mar 7, 2024 •

edited

Loading