On Tue, 6 Feb 2024 22:59:04 GMT, Coleen Phillimore <cole...@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?
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
