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

[David Holmes]

On Thu, 20 Apr 2023 17:37:48 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> I agree we need these ordering constraints, but I'm not clear what is actually guaranteeing them on either the reader or writer side. I imagine there are a number of "barriers" in relation to TLH construction, but as adding to the threadsList is done under a lock (as is setting eetop) then I'm not sure what explicit barriers exist there. Maybe `java_lang_Thread::set_thread` needs to use `release_address_field_put`?
>
> 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).

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

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