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

[Daniel D. Daugherty]

On Thu, 4 May 2023 06:54:37 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Daniel D. Daugherty has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
>> 
>>  - Merge branch 'JDK-8307068' into JDK-8305670
>>  - Merge JDK-8305670 with code review changes in JDK-8307068.
>>  - Merge branch 'JDK-8307068' into JDK-8305670
>>  - Merge branch 'jdk-21+20' into JDK-8305670
>>  - 8305670.with-rehn-quick-mode.cr3.patch
>>  - Use optimized logic in Unsafe_Unpark() instead of cv_internal_thread_to_JavaThread().
>>  - dholmes CR - add more comments.
>>  - 8305670.with-rehn-quick-mode.cr0.patch
>
> src/hotspot/share/runtime/threadSMR.cpp line 847:
> 
>> 845:     // We captured a non-nullptr JavaThread* before the _tlh was created
>> 846:     // so that covers the early life stage of the target JavaThread.
>> 847:     _protected_java_thread = java_lang_Thread::thread_acquire(thread_oop);
> 
> The caller must use `thread_acquire`  so it serves no purpose doing it again here.

I will have to disagree since the initialization and construction of this helper object
is racing with the target JavaThread's termination code path in `ensure_join()`.

The caller's `thread_acquire()` and the `thread_acquire()` on L847 are racing with
this code block in `ensure_join()`:

 // Clear the native thread instance - this makes isAlive return false and allows the join()
  // to complete once we've done the notify_all below. Needs a release() to obey Java Memory Model
  // requirements.
  java_lang_Thread::release_set_thread(threadObj(), nullptr);
  lock.notify_all(thread);
  // Ignore pending exception, since we are exiting anyway
  thread->clear_pending_exception();


If the caller's `thread_acquire()` happens before the call to `release_set_thread()`,
then we'll execute the code block controlled by `L844   if (java_thread != nullptr) {`
and we will get to the `thread_acquire()` call on L847. Let's say that the target
JavaThread has now made it past the `release_set_thread()` call so now the
JavaThread* stored in the java.lang.Thread object is now nullptr. We want the
`thread_acquire()` on L847 to match up with the `release_set_thread()` so that
we see the fact that the JavaThread* stored in the java.lang.Thread object is now
nullptr.

Please let me know if I missed something here.

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

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