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

[Robbin Ehn]

On Thu, 20 Apr 2023 17:50:47 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> 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.

It's also possible to create a special version of ThreadsListHandle which takes an Thread object in constructor.
Do the eetop check and then safe fetch the ThreadsList.
If you call include() with the same JavaThread* as the eetop was before creating the ThreadsList, you can return true directly, i.e. quick mode.


SpecialThreadsListHandle::SpecialThreadsListHandle(oop thread_oop) {
  _javaThread_from_eetop = java_lang_Thread::thread(thread_oop);
  _threadList = ...
}

bool includes(JavaThread* jt) {
  if (jt == _javaThread_from_eetop && jt != nullptr) return true;
  return _threadsList->includes(jt);
}

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

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