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

[Daniel D. Daugherty]

On Fri, 5 May 2023 01:35:03 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> I will have to disagree since the initialization and construction of this helper object
>> is racing with the target JavaThread's termination code path in `ensure_join()`.
>> 
>> The caller's `thread_acquire()` and the `thread_acquire()` on L847 are racing with
>> this code block in `ensure_join()`:
>> 
>>  // Clear the native thread instance - this makes isAlive return false and allows the join()
>>   // to complete once we've done the notify_all below. Needs a release() to obey Java Memory Model
>>   // requirements.
>>   java_lang_Thread::release_set_thread(threadObj(), nullptr);
>>   lock.notify_all(thread);
>>   // Ignore pending exception, since we are exiting anyway
>>   thread->clear_pending_exception();
>> 
>> 
>> If the caller's `thread_acquire()` happens before the call to `release_set_thread()`,
>> then we'll execute the code block controlled by `L844   if (java_thread != nullptr) {`
>> and we will get to the `thread_acquire()` call on L847. Let's say that the target
>> JavaThread has now made it past the `release_set_thread()` call so now the
>> JavaThread* stored in the java.lang.Thread object is now nullptr. We want the
>> `thread_acquire()` on L847 to match up with the `release_set_thread()` so that
>> we see the fact that the JavaThread* stored in the java.lang.Thread object is now
>> nullptr.
>> 
>> Please let me know if I missed something here.
>
> That release in `ensure_join` is for Java volatile semantics for threads calling `is_alive()`. It is there to ensure that any Java writes made by the thread before it terminated are seen by the thread that sees `is_alive()` return false. The VM code we are dealing with here does not need to see any such writes by the terminating thread, nor are there any interesting writes in the VM code which must be seen if `thread()` returned null. The code we are dealing with here only needs to synchronize with the thread startup logic and in particular its addition to the ThreadsList. The `thread_acquire` in the caller ensures that if we see a non-null value then we also see the writes to the ThreadsList as required. 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, hence acquire semantics are not needed.

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.

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

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