RFR: 8307068: store a JavaThread* in the java.lang.Thread object after the JavaThread* is added to the main ThreadsList [v2]

[David Holmes]

On Fri, 28 Apr 2023 19:53:59 GMT, Daniel D. Daugherty <dcubed at openjdk.org> wrote:

>> A small fix to store a JavaThread* in the java.lang.Thread object after the JavaThread*
>> is added to the main ThreadsList. Also adds the following:
>> - `java_lang_Thread::thread_acquire()`
>> - `java_lang_Thread::release_set_thread()`
>> 
>> And uses them on an as-needed basis.
>
> Daniel D. Daugherty has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase.

The eetop field is defined as a Java volatile and so the VM should nominally always perform "volatile" reads and writes ie acquires and releases. But I said nominally, because in many cases there are other synchronization "release" barriers that must have occurred before we could get to the operations that read the eetop field. So I think it is only code that could interact with just started threads, that would need the explicit acquire; while I think any set of the eetop to non-null needs to have release semantics.

src/hotspot/share/runtime/javaThread.cpp line 1675:

> 1673:   // Publish the JavaThread* in java.lang.Thread after the JavaThread* is
> 1674:   // on a ThreadsList. There will be a release when the Theads_lock is
> 1675:   // dropped somewhere in the caller, but let's be more proactive.

Any release associated with dropping the Threads_lock may come too late given this thread is now visible via JVMTI, so this is more than just being "proactive" IMO.

src/hotspot/share/runtime/javaThread.cpp line 2113:

> 2111:   // Publish the JavaThread* in java.lang.Thread after the JavaThread* is
> 2112:   // on a ThreadsList.
> 2113:   java_lang_Thread::set_thread(thread_oop(), target); // isAlive == true now

Why did you decide this doesn't have to be a release?

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

PR Review: https://git.openjdk.org/jdk/pull/13723#pullrequestreview-1408677073
PR Review Comment: https://git.openjdk.org/jdk/pull/13723#discussion_r1182270816
PR Review Comment: https://git.openjdk.org/jdk/pull/13723#discussion_r1182273726