On Thu, 8 Feb 2024 21:30:48 GMT, David Holmes <dhol...@openjdk.org> 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
