RFR: 8231269: CompileTask::is_unloaded is slow due to JNIHandles type checks [v6]

[Vladimir Ivanov]

On Wed, 23 Apr 2025 17:31:07 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> [JDK-8163511](https://bugs.openjdk.org/browse/JDK-8163511) made the `CompileTask` improvement to avoid blocking class unloading if a relevant compile task is in queue. Current code does a sleight-of-hand to make sure the the `method*` in `CompileTask` are still valid before using them. Still a noble goal, so we keep trying to do this.
>> 
>> The code tries to switch weak JNI handle with a strong one when it wants to capture the holder to block unloading. Since we are reusing the same field, we have to do type checks like `JNIHandles::is_weak_global_handle(_method_holder)`. Unfortunately, that type-check goes all the way to `OopStorage` allocation code to verify the handle is really allocated in the relevant `OopStorage`. This takes internal `OopStorage` locks, and thus is slow.
>> 
>> This issue is clearly visible in Leyden, when there are lots of `CompileTask`-s in the queue, dumped by AOT code loader. It also does not help that `CompileTask::select_task` is effectively quadratic in number of methods in queue, so we end up calling `CompileTask::is_unloaded` very often.
>> 
>> It is possible to mitigate this issue by splitting the related fields into weak and strong ones. But as Kim mentions in the bug, we should not be using JNI handles here at all, and instead go directly for relevant `OopStorage`-s. This is what this PR does, among other things that should hopefully make the whole mechanics clearer.
>> 
>> Additional testing:
>>  - [x] Linux x86_64 server fastdebug, `compiler/classUnloading`, 100x still passes; these tests are sensitive to bugs in this code
>>  - [x] Linux x86_64 server fastdebug, `all`
>>  - [x] Linux AArch64 server fastdebug, `all`
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Allow UMH::_method access from VMStructs

Nice work!

src/hotspot/share/runtime/unloadableMethodHandle.hpp line 43:

> 41: //  3. Final released state. Relevant Method* is in unknown state, and cannot be
> 42: //     accessed.
> 43: //

Please, elaborate what state transitions are supported. Currently, my understanding is there are 3 transitions and 4 states:
* 1 -> 2
* 2 -> 3 (terminal)
* 1 -> 3 (terminal)
* 0 (empty, terminal)

src/hotspot/share/runtime/unloadableMethodHandle.inline.hpp line 26:

> 24: 
> 25: #ifndef SHARE_RUNTIME_METHOD_UNLOAD_BLOCKER_HANDLE_INLINE_HPP
> 26: #define SHARE_RUNTIME_METHOD_UNLOAD_BLOCKER_HANDLE_INLINE_HPP

Stale header file name used?

src/hotspot/share/runtime/unloadableMethodHandle.inline.hpp line 37:

> 35: inline UnloadableMethodHandle::UnloadableMethodHandle(Method* method) {
> 36:   _method = method;
> 37:   if (method != nullptr) {

Is it possible to require `method` (and hence `_method`) to always be non-null?

src/hotspot/share/runtime/unloadableMethodHandle.inline.hpp line 57:

> 55: 
> 56:   // Null holder, the relevant class would not be unloaded.
> 57:   return nullptr;

Is this the case of bootstrap classloader? As an optimization opportunity, it can be extended for other system loaders.

src/hotspot/share/runtime/unloadableMethodHandle.inline.hpp line 93:

> 91: 
> 92: inline Method* UnloadableMethodHandle::method() const {
> 93:   assert(!is_unloaded(), "Should not be unloaded");

Assert that `block_unloading()` was called before?

-------------

PR Review: https://git.openjdk.org/jdk/pull/24018#pullrequestreview-2788983703
PR Review Comment: https://git.openjdk.org/jdk/pull/24018#discussion_r2057101817
PR Review Comment: https://git.openjdk.org/jdk/pull/24018#discussion_r2057087135
PR Review Comment: https://git.openjdk.org/jdk/pull/24018#discussion_r2057089091
PR Review Comment: https://git.openjdk.org/jdk/pull/24018#discussion_r2057091706
PR Review Comment: https://git.openjdk.org/jdk/pull/24018#discussion_r2057084091