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

[David Holmes]

On Thu, 4 May 2023 18:59:58 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> src/hotspot/share/runtime/threadSMR.cpp line 847:
>> 
>>> 845:     // We captured a non-nullptr JavaThread* before the _tlh was created
>>> 846:     // so that covers the early life stage of the target JavaThread.
>>> 847:     _protected_java_thread = java_lang_Thread::thread_acquire(thread_oop);
>> 
>> The caller must use `thread_acquire`  so it serves no purpose doing it again here.
>
> 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.

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

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