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

[Daniel D. Daugherty]

On Mon, 8 May 2023 02:43:00 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> I'm sorry, but I will have to disagree again. Here's the first `thread_acquire()` call
>> along with the comment that explains that this first call is for the early life stage of
>> the target JavaThread:
>> 
>>     // Get the JavaThread* stored in the java.lang.Thread object _before_
>>     // the embedded ThreadsListHandle is constructed so we know if the
>>     // early life stage of the JavaThread* is protected.
>>     FastThreadsListHandle ftlh(thread_oop, java_lang_Thread::thread_acquire(thread_oop));
>> 
>> 
>> At this point in the code, all we have done is potentially gather the `JavaThread*`
>> that's stored in the java.lang.Thread object. That value we gathered, might be
>> nullptr and it might be non-nullptr. For the first `thread_acquire` call, I see it
>> as matching up with the `release_set_thread` calls at the end of thread startup.
>> 
>> Here's the helper's constructor code:
>> 
>> FastThreadsListHandle::FastThreadsListHandle(oop thread_oop, JavaThread* java_thread) : _protected_java_thread(nullptr) {
>>   assert(thread_oop != nullptr, "must be");
>>   if (java_thread != nullptr) {
>>     // We captured a non-nullptr JavaThread* before the _tlh was created
>>     // so that covers the early life stage of the target JavaThread.
>>     _protected_java_thread = java_lang_Thread::thread_acquire(thread_oop);
>> 
>> 
>> We check the `java_thread` parameter and if it's non-nullptr, then we know
>> that the `JavaThread*` stored in the java.lang.Thread object was non-nullptr
>> before we constructed the embedded ThreadsListHandle (_tlh). Now we can
>> check the second half of the question...
>> 
>> Here's the second `thread_acquire()` call again along with the assert() and the
>> comment that explains that this second call is for end life stage of the target
>> `JavaThread*`.
>> 
>>     _protected_java_thread = java_lang_Thread::thread_acquire(thread_oop);
>>     assert(_protected_java_thread == nullptr || _tlh.includes(_protected_java_thread), "must be");
>>     // If we captured a non-nullptr JavaThread* after the _tlh was created
>>     // then that covers the end life stage of the target JavaThread and we
>>     // we know that _tlh protects the JavaThread*.
>> 
>> 
>>> The subsequent re-reading of that value will return null or non-null but it does
>>> not need to synchronize with any other memory writes
>> 
>> The second `thread_acquire` most certainly does need to synchronize with the
>> code in `ensure_join()` that does a `release_set_thread` of a nullptr. If that code
>> set a nullptr before we could read a non-nullptr value a second time, then we can
>> make no inferences about what `_tlh` contains. So I see the second `thread_acquire`
>> as matching up with the `release_set_thread` in `ensure_join`.
>> 
>> If the `_protected_java_thread` field is nullptr, then we don't know that `_tlh` has
>> captured the target `JavaThread*` and the `JavaThread*` is NOT protected.
>> If `_protected_java_thread` != nullptr, then we do know that `_tlh` contains the
>> target `JavaThread*` and `JavaThread*` is protected.
>
>> The second `thread_acquire` most certainly does need to synchronize with the
>> code in `ensure_join()` that does a `release_set_thread` of a nullptr. If that code
>> set a nullptr before we could read a non-nullptr value a second time, then we can
>> make no inferences about what `_tlh` contains. So I see the second thread_acquire
>> as matching up with the release_set_thread in ensure_join.
> 
> The only memory value of interest is the read of eetop. The release/acquire have no affect on the reading or writing of that field itself. We would only need acquire semantics when reading the potentially null eetop if we were then to read another memory value that would be written prior to eetop being set to null. That is not the case hence the acquire semantics are unnecessary.

I've read thru this thread of comments three times and I think I see where and how my
brain got confused by what we are doing here. Let me see if I can unwind this mess that
I have made...

Our new `FastThreadsListHandle` helper object is used by `Unsafe_Unpark()` to quickly
determine whether our target thread (`jthread` parameter) is protected by the embedded
`_tlh` in the helper object. The `Unsafe_Unpark()` call is racing against two different
situations in the target thread's life-cycle:

1) The first situation is the early life stage of the target thread. It is during this stage that
    the target `JavaThread*` gets added to the main ThreadsList AND the `JavaThread*`
    gets stored in the java.lang.Thread object.

    From the `Unsafe_Unpark()` callers POV, we need to see two things happen: the target
    `JavaThread*` getting added to the main ThreadsList and a non-nullptr value being
    stored in the java.lang.Thread object. Seeing both memory operations requires that we
    use `java_lang_Thread::thread_acquire()` for the query in `Unsafe_Unpark()`.

2) The second situation is the end life stage of the target thread. It is during this stage that
     `nullptr` gets stored in the java.lang.Thread object. The target `JavaThread*` will also
     get removed from the main ThreadsList later, but we don't care about that part since the
     setting of the `nullptr` in the java.lang.Thread object is a perfect bailout sentinel.

     From the `FastThreadsListHandle` helper object's POV, we only care about whether we
     see a non-`nullptr` value or a `nullptr` value. Since we only care about a single memory
     operation, we use `java_lang_Thread::thread()` which is implemented with an underlying
     atomic load operation. Acquire semantics are not needed for this query.

Okay so those are the two situations of interest in the target thread. Now let's talk about
`Unsafe_Unpark()`, its use of the `FastThreadsListHandle` helper object and how that
applies to the target thread.

1) The first query of the `JavaThread*` in the java.lang.Thread object that is made in
    `Unsafe_Unpark()` is trying to determine if the target thread is past the early life stage.
    A non-`nullptr` value means that the `_tlh` embedded in the FastThreadsListHandle
    _might_ be protecting the `JavaThread*`. Since this code is primarily targeted at the
    early life stage, we have to use `java_lang_Thread::thread_acquire()` in order to get
    a proper answer from that code path in the target thread.

    Of course, this code might also execute when the target thread is in the end life stage,
    but we'll still get a valid answer from it so no harm, no foul.

2) `Unsafe_Unpark()` passes that first `JavaThread*` value to the FastThreadsListHandle
     helper object and that constructor does the remainder of the work if the value passed
     in is non-`nullptr`. The constructor does the second query of the `JavaThread*` in the
     java.lang.Thread object and it only cares about whether we see a `nullptr` value or a
     non-`nullptr` value so we can use just `java_lang_Thread::thread()` here to fetch the
     value. A non-`nullptr` value means that the embedded `_tlh` does protect the
     `JavaThread*` and `Unsafe_Unpark()` can proceed to use that `JavaThread*` to do
     its work. A `nullptr` value means that the target `JavaThread*` has exited enough to
     make it past ensure_join() where the `JavaThread*` is cleared from the
     java.lang.Thread object.

     This second query does not execute if the target thread is in the early life stage and a
     `nullptr` value is returned by the first query so there is no opportunity for the
     `java_lang_Thread::thread()` query to return an invalid answer from an early life stage.

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

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