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

[David Holmes]

On Tue, 18 Apr 2023 21:09:54 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.

Sorry Dan but it seems to me that if quick_mode is true then we don't even need to call `cv_internal_thread_to_JavaThread` in the first place.

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

> 785:   if (jthread != nullptr) {
> 786:     oop thread_oop = JNIHandles::resolve_non_null(jthread);
> 787:     if (java_lang_Thread::thread(thread_oop) != nullptr) {

Suggestion for a comment after this line:

// We initially have a live JavaThread, so try to capture it in a ThreadsListHandle

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

> 786:     oop thread_oop = JNIHandles::resolve_non_null(jthread);
> 787:     if (java_lang_Thread::thread(thread_oop) != nullptr) {
> 788:       ThreadsListHandle tlh; // Provides memory barrier

I don't understand what the comment means - what kind of memory barrier?

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

> 787:     if (java_lang_Thread::thread(thread_oop) != nullptr) {
> 788:       ThreadsListHandle tlh; // Provides memory barrier
> 789:       if (java_lang_Thread::thread(thread_oop) != nullptr) {

Suggestion for comment after this line:

// We still have a live JavaThread, now protected by the ThreadsListHandle.

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

> 792:         // before and after creating tlh above so quick_mode can be used in
> 793:         // this conversion call.
> 794:         if (tlh.cv_internal_thread_to_JavaThread(jthread, &thr, nullptr, true /* quick_mode */)) {

Sorry I'm having a mental blank here now. Why do we even need to call this if we can access `java_lang_Thread::thread(thread_oop)` after the TLH was created?

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

PR Review: https://git.openjdk.org/jdk/pull/13519#pullrequestreview-1391917758
PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1171246822
PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1171244811
PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1171248149
PR Review Comment: https://git.openjdk.org/jdk/pull/13519#discussion_r1171250932