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

[Daniel D. Daugherty]

On Fri, 21 Apr 2023 02:41:41 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> There are ~76 ThreadsListHandle variables in the code base. These are the only ones with comments:
>> 
>> src/hotspot/share/runtime/threadSMR.cpp:    ThreadsListHandle tlh;  // make the current ThreadsList safe for reporting
>> src/hotspot/share/prims/unsafe.cpp:      ThreadsListHandle tlh; // Provides memory barrier
>> src/hotspot/share/compiler/compileBroker.cpp:        ThreadsListHandle tlh;  // name() depends on the TLH.
>> src/hotspot/share/compiler/compileBroker.cpp:        ThreadsListHandle tlh;  // name() depends on the TLH.
>> src/hotspot/share/compiler/compileBroker.cpp:        ThreadsListHandle tlh;  // name() depends on the TLH.
>> src/hotspot/share/compiler/compileBroker.cpp:        ThreadsListHandle tlh;  // name() depends on the TLH.
>> 
>> 
>> The new "// Provides memory barrier" is the only one that even talks about barriers.
>> I'm inclined to delete the comment given that these current lines of code:
>> 
>>     if (java_lang_Thread::thread(thread_oop) != nullptr) {
>>       // Try to capture the live JavaThread in a ThreadsListHandle:
>>       ThreadsListHandle tlh;
>>       if (java_lang_Thread::thread(thread_oop) != nullptr) {
>>         // The still live JavaThread is protected by the ThreadsListHandle.
>>         JavaThread* thr = nullptr;
>> 
>> I think the comments added in the previous comment are enough to make this code clear.
>> @robehn and @dholmes-ora - are you both good with this?
>
> I don't mind if the "memory barrier" comment is there or not. I'm more concerned about whether the actual required ordering constraints are in fact enforced. I think `java_lang_Thread::set_thread` may need to have release semantics (and the matching `acquire` is (implicitly) within the TLH construction).

@dholmes-ora - I'm sorry that I forgot to reply to this earlier comment in this thread:
https://github.com/openjdk/jdk/pull/13519#discussion_r1171987653

> but as adding to the threadsList is done under a lock (as is setting eetop) 

Adding a new JavaThread* to the main ThreadsList is done under a lock.
The `java_lang_Thread::set_thread()` calls are less clean:
- there's one call from`create_initial_thread()` that doesn't have the Threads_lock, but I also don't think it needs it.
- there's one call from `JavaThread::allocate_threadObj()` which happens during JNI attach that doesn't have the Threads_lock, but I don't think it needs it

- the call from `JavaThread::prepare()` has the Threads_lock
- the call from `JavaThread::start_internal_daemon()` has the Threads_lock

Of course, here's our favorite call from `ensure_join()`:

static void ensure_join(JavaThread* thread) {
  // We do not need to grab the Threads_lock, since we are operating on ourself.
<snip>
  OrderAccess::release();
  java_lang_Thread::set_thread(threadObj(), nullptr);


Obviously someone thought that call to `java_lang_Thread::set_thread()` needed a "release".

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

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