RFR: 8305670: Performance regression in LockSupport.unpark with lots of idle threads [v2]

[Daniel D. Daugherty]

On Thu, 20 Apr 2023 00:58:45 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> In particular we want this assert from `cv_internal_thread_to_JavaThread()`:
>> 
>>   assert(includes(java_thread), "must be");
>> 
>> because that verifies our assumptions from this call-site that `quick_mode` is okay.
>> 
>> Also, it is possible that a JavaThread that's racing to exit will make it past `ensure_join()`
>> after our check on L789 above and run into this block in the conversion function:
>> 
>>   JavaThread *java_thread = java_lang_Thread::thread(thread_oop);
>>   if (java_thread == nullptr) {
>>     // The java.lang.Thread does not contain a JavaThread* so it has not
>>     // run enough to be put on a ThreadsList or it has exited enough to
>>     // make it past ensure_join() where the JavaThread* is cleared.
>>     return false;
>>   }
>> 
>> 
>> Because we (the observer thread) captured the target JavaThread in the
>> ThreadsListHandle, it's safe for us to use the `JavaThread*`, but I've always
>> been a fan of bailing on operations like these as soon as we detect that the
>> `JavaThread*` is exiting.
>
> But ignoring the asserts `cv_internal_thread_to_JavaThread()` serves absolutely no purpose when we could request quick-mode - and we don't need to know what `cv_internal_thread_to_JavaThread` does - we know how thread startup and exit is managed and we know what a TLH guarantees . Sure we can bail sooner but there is no need to, so that is not required. In quick-mode, barring the assertion for includes, it all boils down to nothing! We don't need the Thread_oop passed out as we already have it before we start; we don't even need the JavaThread* passed out as we already have it too! So what does it actually do for us? Nothing. We could just have:
> 
> UNSAFE_ENTRY(void, Unsafe_Unpark(JNIEnv *env, jobject unsafe, jobject jthread)) {
>   if (jthread != nullptr) {
>     oop thread_oop = JNIHandles::resolve_non_null(jthread);
>     if (java_lang_Thread::thread(thread_oop) != nullptr) {
>       // Try to capture the live JavaThread in a ThreadsListHandle:
>       ThreadsListHandle tlh; // Provides memory barrier
>       JavaThread* thr;
>       if ( (thr = java_lang_Thread::thread(thread_oop)) != nullptr) {
>         assert(tlh.includes(thr), "must be");
>         // The still live JavaThread is protected by the ThreadsListHandle
>         // so is safe to access.
>         Parker* p = thr->parker();
>         HOTSPOT_THREAD_UNPARK((uintptr_t) p);
>         p->unpark();
>       }
>     } // ThreadsListHandle is destroyed here.
>   }
> } UNSAFE_END

Before we went down the road of creating this `quick_mode` variant of the
conversion function, I added this comment about "API indigestion" to the bug report:

https://bugs.openjdk.org/browse/JDK-8305670?focusedCommentId=14573647&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14573647

@robehn's proposed `quick_mode` changes were an attempt to make the indigestion
a little less bothersome (Tums anyone?), but @dholmes-ora's comments have made a
good case here for what we've done is jump thru hoops to keep a call to the (slightly
modified) conversion function when it's clear that the general purpose conversion
function just isn't the right tool for this particular job.

I'm going to take another look at backing out portions of the `quick_mode` fix while
trying to keep the performance improvements. This will result in `Unsafe_Unpark()`
not trying to use the general purpose conversion function.

On a related note, we're having a bit of trouble trying to reproduce the performance
improvements on the machines in our performance lab.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1172918536