Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
On Fri, 8 Mar 2024 06:17:06 GMT, David Holmes wrote:
>> Coleen Phillimore has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Missed a word.
>> - David's comment fixes.
>
> src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 121:
>
>> 119: {
>> 120: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 121: // 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1517716370
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
On Thu, 7 Mar 2024 13:21:09 GMT, Coleen Phillimore wrote: >> 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. > > Coleen Phillimore has updated the pull request incrementally with two > additional commits since the last revision: > > - Missed a word. > - David's comment fixes. Thank you for reviewing, Dean, David and Fred. - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1985694821
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
On Thu, 7 Mar 2024 13:21:09 GMT, Coleen Phillimore wrote:
>> 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.
>
> Coleen Phillimore has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Missed a word.
> - David's comment fixes.
Marked as reviewed by dholmes (Reviewer).
src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 121:
> 119: {
> 120: // To get a consistent list of classes we need MultiArray_lock to
> ensure
> 121: // 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.
-
PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1924097971
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1517209655
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
On Thu, 7 Mar 2024 13:21:09 GMT, Coleen Phillimore wrote: >> 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. > > Coleen Phillimore has updated the pull request incrementally with two > additional commits since the last revision: > > - Missed a word. > - David's comment fixes. LGTM - Marked as reviewed by fparain (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1923623357
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:44:22 GMT, David Holmes wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Dean's comments.
>
> src/hotspot/share/oops/instanceKlass.cpp line 1552:
>
>> 1550: RecursiveLocker rl(MultiArray_lock, THREAD);
>> 1551:
>> 1552: // This thread is the creator.
>
> This thread may not be the creator. The original comment was more apt - we
> typically say something like "Check if another thread beat us to it."
Fixed.
// Check if another thread created the array klass while we were waiting
for the lock.
> src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 121:
>
>> 119: {
>> 120: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 121: // array classes aren't created during this walk. This walks
>> through the
>
> Just curious but how can walking the set of classes trigger creation of array
> classes?
It can't. I'll reword that comment so it's clearer, with just a couple of
words.
// To get a consistent list of classes we need MultiArray_lock to ensure
// array classes aren't created by another thread this walk. This walks
through the
// InstanceKlass::_array_klasses links.
edit: added "during" to "by another thread during this walk"
> src/hotspot/share/runtime/mutexLocker.hpp line 335:
>
>> 333:
>> 334: // Instance of a RecursiveLock that may be held through Java heap
>> allocation, which may include calls to Java,
>> 335: // and JNI event notification for resource exhausted for metaspace or
>> heap.
>
> s/exhausted/exhaustion/
Fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1516126218
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1516128248
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1516130552
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
On Thu, 7 Mar 2024 01:31:09 GMT, Coleen Phillimore wrote:
>> 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1516129405
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:38:56 GMT, Coleen Phillimore wrote: >> 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. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. David, thank you for your discussion while fixing this problem. - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1922394063
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v3]
> 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. Coleen Phillimore has updated the pull request incrementally with two additional commits since the last revision: - Missed a word. - David's comment fixes. - Changes: - all: https://git.openjdk.org/jdk/pull/17739/files - new: https://git.openjdk.org/jdk/pull/17739/files/d64aa560..88fd6f13 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17739=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17739=01-02 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/17739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17739/head:pull/17739 PR: https://git.openjdk.org/jdk/pull/17739
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:38:56 GMT, Coleen Phillimore wrote:
>> 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.
>
> Coleen Phillimore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Dean's comments.
This looks good to me. Thanks for working through the details with me.
src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 121:
> 119: {
> 120: // To get a consistent list of classes we need MultiArray_lock to
> ensure
> 121: // array classes aren't created during this walk. This walks through
> the
Just curious but how can walking the set of classes trigger creation of array
classes?
src/hotspot/share/runtime/mutexLocker.hpp line 335:
> 333:
> 334: // Instance of a RecursiveLock that may be held through Java heap
> allocation, which may include calls to Java,
> 335: // and JNI event notification for resource exhausted for metaspace or
> heap.
s/exhausted/exhaustion/
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1921323899
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515402581
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515406082
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:38:56 GMT, Coleen Phillimore wrote: >> 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. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. > 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. - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1982207873
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:38:56 GMT, Coleen Phillimore wrote: >> 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. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. src/hotspot/share/oops/instanceKlass.cpp line 1552: > 1550: RecursiveLocker rl(MultiArray_lock, THREAD); > 1551: > 1552: // This thread is the creator. This thread may not be the creator. The original comment was more apt - we typically say something like "Check if another thread beat us to it." - PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515371571
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 00:54:30 GMT, David Holmes wrote:
>> 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515364466
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 00:18:53 GMT, Dean Long wrote:
>> Coleen Phillimore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Dean's comments.
>
> src/hotspot/share/oops/arrayKlass.cpp line 135:
>
>> 133: ResourceMark rm(THREAD);
>> 134: {
>> 135: // Ensure atomic creation of higher dimensions
>
> Isn't this comment still useful?
Yes, it's a useful comment, readded.
> src/hotspot/share/oops/arrayKlass.cpp line 144:
>
>> 142: ObjArrayKlass* ak =
>> 143: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
>> dim + 1, this, CHECK_NULL);
>> 144: ak->set_lower_dimension(this);
>
> Would it be useful to assert that the lower dimension has been set?
Yes, added.
> src/hotspot/share/oops/instanceKlass.cpp line 2765:
>
>> 2763: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 2764: // array classes aren't observed while they are being restored.
>> 2765: MutexLocker ml(MultiArray_lock);
>
> Doesn't this revert JDK-8274338?
You're right. I didn't believe my own comment when I removed this. Thank you
for pointing out the bug.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515346198
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515346716
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515349878
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
On Thu, 7 Mar 2024 01:35:45 GMT, Coleen Phillimore wrote: >> 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. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Dean's comments. 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. - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1921122133
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock [v2]
> 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. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Dean's comments. - Changes: - all: https://git.openjdk.org/jdk/pull/17739/files - new: https://git.openjdk.org/jdk/pull/17739/files/3625e6cc..d64aa560 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17739=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17739=00-01 Stats: 5 lines in 2 files changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17739.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17739/head:pull/17739 PR: https://git.openjdk.org/jdk/pull/17739
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Thu, 7 Mar 2024 00:16:39 GMT, Dean Long wrote: >> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515343108
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote: > 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. Marked as reviewed by dlong (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1921100707
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote: > 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. src/hotspot/share/oops/instanceKlass.cpp line 2765: > 2763: // To get a consistent list of classes we need MultiArray_lock to > ensure > 2764: // array classes aren't observed while they are being restored. > 2765: MutexLocker ml(MultiArray_lock); Doesn't this revert JDK-8274338? - PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515330292
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote:
> 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.
src/hotspot/share/oops/arrayKlass.cpp line 135:
> 133: ResourceMark rm(THREAD);
> 134: {
> 135: // Ensure atomic creation of higher dimensions
Isn't this comment still useful?
src/hotspot/share/oops/arrayKlass.cpp line 144:
> 142: ObjArrayKlass* ak =
> 143: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
> dim + 1, this, CHECK_NULL);
> 144: ak->set_lower_dimension(this);
Would it be useful to assert that the lower dimension has been set?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515322667
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515323404
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Wed, 6 Mar 2024 23:47:01 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/mutex.cpp line 537:
>>
>>> 535: // can be called by jvmti by VMThread.
>>> 536: if (current->is_Java_thread()) {
>>> 537: _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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515314037
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Wed, 6 Mar 2024 23:09:34 GMT, Dean Long wrote:
>> 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.
>
> src/hotspot/share/runtime/mutex.cpp line 537:
>
>> 535: // can be called by jvmti by VMThread.
>> 536: if (current->is_Java_thread()) {
>> 537: _sem.wait_with_safepoint_check(JavaThread::cast(current));
>
> Why not use PlatformMutex + OSThreadWaitState instead of a semaphore?
Semaphore seemed simpler (?)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515294583
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote:
> 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.
OK, that makes sense about loom and JOM.
src/hotspot/share/runtime/mutex.cpp line 537:
> 535: // can be called by jvmti by VMThread.
> 536: if (current->is_Java_thread()) {
> 537: _sem.wait_with_safepoint_check(JavaThread::cast(current));
Why not use PlatformMutex + OSThreadWaitState instead of a semaphore?
-
PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1982008253
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1515269443
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 5 Mar 2024 23:13:30 GMT, Dean Long wrote: >> 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. > > 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. @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. - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1980812019
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 5 Mar 2024 23:13:30 GMT, Dean Long wrote: > 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. ?? - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1980207637
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote: > 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. 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. - PR Comment: https://git.openjdk.org/jdk/pull/17739#issuecomment-1979796523
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 13 Feb 2024 02:07:54 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/oops/arrayKlass.cpp line 141:
>>
>>> 139: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
>>> dim + 1, this, CHECK_NULL);
>>> 140: // use 'release' to pair with lock-free load
>>> 141: release_set_higher_dimension(ak);
>>
>> Why has this code changed? I only expected to see the lock changed.
>
> The assert is dumb, leftover from when we didn't have C++ types (only
> klassOop). Of course it's an objArrayKlass, that's its type! The higher
> dimension should be set in the constructor of ObjArrayKlass. Every version
> of this change, I move this assignment there.
Changing this like this makes it more similar to the InstanceKlass::array_klass
function in structure, and there was unnecessary { } scopes in both and an
unnecessary ResourceMark.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487058776
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Thu, 8 Feb 2024 21:30:48 GMT, David Holmes wrote:
>> 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-4.
>
> src/hotspot/share/classfile/classLoaderData.cpp line 412:
>
>> 410: // To call this, one must have the MultiArray_lock held, but the
>> _klasses list still has lock free reads.
>> 411: assert_locked_or_safepoint(MultiArray_lock);
>> 412:
>
> Do we no longer hold the lock or are you just missing the API to assert it is
> held?
We no longer hold the lock in the callers to iterate through CLD::_klasses
list. It was never needed. CLD::_klasses has iterators that are lock free
(atomics)
> src/hotspot/share/oops/arrayKlass.cpp line 141:
>
>> 139: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
>> dim + 1, this, CHECK_NULL);
>> 140: // use 'release' to pair with lock-free load
>> 141: release_set_higher_dimension(ak);
>
> Why has this code changed? I only expected to see the lock changed.
The assert is dumb, leftover from when we didn't have C++ types (only
klassOop). Of course it's an objArrayKlass, that's its type! The higher
dimension should be set in the constructor of ObjArrayKlass. Every version of
this change, I move this assignment there.
> src/hotspot/share/prims/jvmtiExport.cpp line 3151:
>
>> 3149: if (MultiArray_lock->owner() == thread) {
>> 3150: return false;
>> 3151: }
>
> So the recursive nature of the lock now means we don't have to bail out here
> - right?
Yes. If it were only the JNI upcall that was broken while holding the
MultiArray_lock, I think I could have used this same approach. Unfortunately,
its also throwing OOM for metaspace and for Java heap :(
> src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 107:
>
>> 105: // To get a consistent list of classes we need MultiArray_lock to
>> ensure
>> 106: // array classes aren't created.
>> 107: MutexLocker ma(MultiArray_lock);
>
> Unclear why this is no longer the case.
CLD::_klasses list is iterated through lock free with atomics. Older code used
to iterate through the system dictionary which only had InstanceKlasses and
then used to iterate through the arrays in InstanceKlass::_array_klasses.
Which required the MultiArray_lock protecting it. There used to be an
array_klasses_do() function. Now all the klasses are in the CLD::_klasses
list and traversed atomically.
> src/hotspot/share/runtime/recursiveLock.cpp line 45:
>
>> 43: _recursions++;
>> 44: assert(_recursions == 1, "should be");
>> 45: Atomic::release_store(&_owner, current); // necessary?
>
> No release necessary. The only thing written since the sem.wait is recursions
> and it is only updated or needed by the owning thread.
ok thanks.
> src/hotspot/share/runtime/recursiveLock.cpp line 54:
>
>> 52: _recursions--;
>> 53: if (_recursions == 0) {
>> 54: Atomic::release_store(&_owner, (Thread*)nullptr); // necessary?
>
> No release necessary.
ok, thanks.
> src/hotspot/share/runtime/recursiveLock.hpp line 33:
>
>> 31: // There are also no checks that the recursive lock is not held when
>> going to Java or to JNI, like
>> 32: // other JVM mutexes have. This should be used only for cases where the
>> alternatives with all the
>> 33: // nice safety features don't work.
>
> Mention that it does interact with safepoints properly for JavaThreads
Added a comment.
> src/hotspot/share/runtime/recursiveLock.hpp line 45:
>
>> 43: void unlock(Thread* current);
>> 44: };
>> 45:
>
> Should expose a `holdsLock` method to allow use in assertions.
Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487056926
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487058194
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487060684
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487062202
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487063071
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1487063156
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1492431178
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1492430934
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote:
> 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-4.
src/hotspot/share/classfile/classLoaderData.cpp line 412:
> 410: // To call this, one must have the MultiArray_lock held, but the
> _klasses list still has lock free reads.
> 411: assert_locked_or_safepoint(MultiArray_lock);
> 412:
Do we no longer hold the lock or are you just missing the API to assert it is
held?
src/hotspot/share/oops/arrayKlass.cpp line 141:
> 139: ObjArrayKlass::allocate_objArray_klass(class_loader_data(),
> dim + 1, this, CHECK_NULL);
> 140: // use 'release' to pair with lock-free load
> 141: release_set_higher_dimension(ak);
Why has this code changed? I only expected to see the lock changed.
src/hotspot/share/prims/jvmtiExport.cpp line 3151:
> 3149: if (MultiArray_lock->owner() == thread) {
> 3150: return false;
> 3151: }
So the recursive nature of the lock now means we don't have to bail out here -
right?
src/hotspot/share/prims/jvmtiGetLoadedClasses.cpp line 107:
> 105: // To get a consistent list of classes we need MultiArray_lock to
> ensure
> 106: // array classes aren't created.
> 107: MutexLocker ma(MultiArray_lock);
Unclear why this is no longer the case.
src/hotspot/share/runtime/recursiveLock.cpp line 45:
> 43: _recursions++;
> 44: assert(_recursions == 1, "should be");
> 45: Atomic::release_store(&_owner, current); // necessary?
No release necessary. The only thing written since the sem.wait is recursions
and it is only updated or needed by the owning thread.
src/hotspot/share/runtime/recursiveLock.cpp line 54:
> 52: _recursions--;
> 53: if (_recursions == 0) {
> 54: Atomic::release_store(&_owner, (Thread*)nullptr); // necessary?
No release necessary.
src/hotspot/share/runtime/recursiveLock.hpp line 33:
> 31: // There are also no checks that the recursive lock is not held when
> going to Java or to JNI, like
> 32: // other JVM mutexes have. This should be used only for cases where the
> alternatives with all the
> 33: // nice safety features don't work.
Mention that it does interact with safepoints properly for JavaThreads
src/hotspot/share/runtime/recursiveLock.hpp line 45:
> 43: void unlock(Thread* current);
> 44: };
> 45:
Should expose a `holdsLock` method to allow use in assertions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483613181
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483614823
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483623614
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483624460
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483608746
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483609712
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483610648
PR Review Comment: https://git.openjdk.org/jdk/pull/17739#discussion_r1483612492
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore wrote: > 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-4. Thanks for looking at this WIP. - PR Review: https://git.openjdk.org/jdk/pull/17739#pullrequestreview-1876761135
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Thu, 1 Feb 2024 05:22:24 GMT, David Holmes wrote: >> This change uses a claim token to allocate multi dimensional arrays rather >> than holding MultiArray_lock around metaspace allocation. We can't hold a >> mutex around metaspace allocation because it can create an OOM object and it >> can also call into JVMTI for a resource exhausted event. Also, we were >> creating mirrors and more metadata arrays while holding this lock. See the >> bug for more details and other ideas considered and rejected. >> >> Tested with tier1-7. > > ~~Can't you just use a Monitor to implement the claim token, rather than this > lock-free approach? (Similar to how class initialization is handled.)~~ > > Sorry lost the forest in the trees. I'm going to do a bit of rework as discussed with @dholmes-ora . - PR Comment: https://git.openjdk.org/jdk/pull/17660#issuecomment-1929599218
Re: RFR: 8308745: ObjArrayKlass::allocate_objArray_klass may call into java while holding a lock
On Wed, 31 Jan 2024 18:57:23 GMT, Coleen Phillimore wrote: > This change uses a claim token to allocate multi dimensional arrays rather > than holding MultiArray_lock around metaspace allocation. We can't hold a > mutex around metaspace allocation because it can create an OOM object and it > can also call into JVMTI for a resource exhausted event. Also, we were > creating mirrors and more metadata arrays while holding this lock. See the > bug for more details and other ideas considered and rejected. > > Tested with tier1-7. Can't you just use a Monitor to implement the claim token, rather than this lock-free approach? (Similar to how class initialization is handled.) - PR Review: https://git.openjdk.org/jdk/pull/17660#pullrequestreview-1855472920
