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

[Daniel D. Daugherty]

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

>> 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".

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.

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

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