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

[David Holmes]

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

>> @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".
>
> And I need to address part of this comment:
> https://github.com/openjdk/jdk/pull/13519#discussion_r1173249806
> 
>> I think java_lang_Thread::set_thread may need to have release semantics
>> (and the matching acquire is (implicitly) within the TLH construction).
> 
> I agree we should put release semantics in `java_lang_Thread::set_thread()` and
> I'll work on that next.
> 
> This part is puzzling me:
>> the matching acquire is (implicitly) within the TLH construction
> 
> TLH construction does nothing with the `JavaThread*` stored in the `java.lang.Thread` object
> unless I'm really missing something here. I would think the matching acquire would be in
> any call to `java_lang_Thread::thread()`. 
> 
> So I also think that `java_lang_Thread::thread()` needs to use `address_field_acquire()`
> unless I'm missing something.

Yes you are right that makes things simpler and much neater. We end up with two acquires in the path:

Read eetop
Read threadslist
Read eetop again

but that is okay.

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

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