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

[David Holmes]

On Fri, 21 Apr 2023 20:53:44 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> Address the performance regression in `LockSupport.unpark()` with the following changes:
>> - Add a `quick_mode` parameter to `ThreadsListHandle::cv_internal_thread_to_JavaThread()` that tells the conversion function to skip searching the ThreadsList for the target `JavaThread*`.
>> - Update `Unsafe_Unpark()` to verify that `java_lang_Thread::thread(thread_oop) != nullptr` both before and after the creation of the ThreadsListHandle; this verification that a `JavaThread*` is saved in the `java.lang.Thread` object allows us to know that the target `JavaThread*` is protected by the ThreadsListHandle without searching it.
>> - Move the storing of the `JavaThread*` in the `java.lang.Thread` object until after the `JavaThread*` is added to the system ThreadsList.
>> - This change in the order permits the check of `java_lang_Thread::thread(thread_oop) != nullptr` before the creation of the ThreadsListHandle to have the meaning that we need for a `JavaThread*` that is in the early stages of running.
>> - The check `java_lang_Thread::thread(thread_oop) != nullptr` that happens after the creation of the ThreadsListHandle permits the meaning that we need for a `JavaThread*` that is in the end stages of running.
>> - So the combination of the two `java_lang_Thread::thread(thread_oop) != nullptr` checks allows us to safely skip the ThreadsList search in `ThreadsListHandle::cv_internal_thread_to_JavaThread()`.
>> 
>> Additional changes:
>> - Remove the `EnableThreadSMRExtraValidityChecks` option since that option implies that the ThreadsList search is optional in all situations which is not the case. Also remove comments that mention the option.
>> - Update the comments in `ThreadsListHandle::cv_internal_thread_to_JavaThread()` to clarify what `java_lang_Thread::thread(thread_oop) != nullptr` means in the JavaThread's lifecycle and that function's attempts to convert a `jthread` into a `JavaThread*` that's known to be protected by the ThreadsListHandle.
>
> Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8305670.with-rehn-quick-mode.cr3.patch

src/hotspot/share/classfile/javaClasses.cpp line 1573:

> 1571: void java_lang_Thread::set_thread(oop java_thread, JavaThread* thread) {
> 1572:   java_thread->release_address_field_put(_eetop_offset, (address)thread);
> 1573: }

There is a convention that when we have acquire/release semantics, that we make that evident in the name of the method so that it is obvious at the call-site. So these would need renaming to `thread_acquire` and `release_set_thread`.

src/hotspot/share/prims/unsafe.cpp line 791:

> 789:     // early life stage of the JavaThread* is protected.
> 790:     FastThreadsListHandle ftlh(thread_oop, java_lang_Thread::thread(thread_oop));
> 791:     JavaThread* thr = ftlh.protected_java_thread();

I find this obscures things more than it aids understanding in this case. You don't need a special FTLH to get the desired semantics here.

src/hotspot/share/runtime/javaThread.cpp line 744:

> 742:   // allows the join() to complete once we've done the notify_all() below.
> 743:   // Needs a release() to obey Java Memory Model requirements (which is done
> 744:   // in set_thread()).

With the suggested renaming you don't need to add comments like this.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1178552450
PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1178555965
PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1178553640