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

[Aleksey Shipilev]

On Thu, 8 May 2025 14:29:56 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> src/hotspot/share/compiler/compileBroker.cpp line 1697:
>> 
>>> 1695:   JavaThread* thread = JavaThread::current();
>>> 1696: 
>>> 1697:   methodHandle method(thread, task->method());
>> 
>> I think this is safe because the Method* is in the CompileTask and redefinition will find it there. Being unsure of this is why this is here in a handle.
>
> Ah, that reminds me, thanks.
> 
> I removed this because I caught method to be in unsafe (unloaded) state, so `method()` asserted on me. `compiler/c1/TestConcurrentPatching.java` seems to intermittently crash on it. On this code path, I think we might be plausibly waiting on unloaded compile task, and we "only" wait for notification that task got purged from the queue. Handelizing broken `Method*` is awkward, to say the least!
> 
> Then again, I am not sure if removing this handle is safe enough. So out of abundance of caution, we can actually handelize `Method*` after checking for task status. But now that I do this:
> 
> 
>   methodHandle method(thread, task->is_unloaded() ? nullptr : task->method());
> 
> 
> ...the test still fails on the same assert! Which makes no sense to me, as we are supposed to be guarded by `is_unloaded` check before it. Something is off, I'll investigate.

I understand now. There are TOCTOU-s under concurrent `block_unloading`.

The most egregious one is here: `is_unloaded` checks in two steps: `!_weak_handle.is_empty() && _weak_handle.peek() == nullptr;`. So when `block_unloading` comes in concurrently and resets weak to empty (since we have strong handle now), it might be possible that first predicate is still `true`, but evaluation of second predicate calls `peek` on empty `_weak_handle`, oops. 

We could technically claim that `UnloadableMethodHandle` is not thread-safe, but it does not solve current compiler uses, and it is very unsatisfactory for the utility class. I'll look into ways to make it resilient under concurrent updates.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24018#discussion_r2081467353