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

[David Holmes]

On Wed, 19 Apr 2023 17:20:28 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> As a caller of cv_internal_thread_to_JavaThread we do not know the implementation of quick_mode.
>> 
>> As the implementation is now, we still want the asserts.
>
> 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

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

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