RFR: 8319935: Ensure only one JvmtiThreadState is create for one JavaThread associated with attached native thread

[Jiangli Zhou]

On Mon, 13 Nov 2023 23:21:56 GMT, Man Cao <manc at openjdk.org> wrote:

>> Please review JvmtiThreadState::state_for_while_locked change to handle the state->get_thread_oop() null case. Please see https://bugs.openjdk.org/browse/JDK-8319935 for details.
>
> src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 95:
> 
>> 93:   // the allocation of the thread oop when a native thread is attaching. Make
>> 94:   // sure we don't create a new state for the JavaThread.
>> 95:   if (state == nullptr || (state->get_thread_oop() != nullptr &&
> 
> In my limited testing with and without virtual threads, when `state` is not null, `state->get_thread_oop()` is always either null or equal to `thread_oop`. So I suspect this whole if is equivalent to `if (state == nullptr)` in practice.
> 
> I think it is better to split this if into two conditions. Here is my proposed change: https://github.com/openjdk/jdk/commit/00ace66c36243671a0fb1b673b3f9845460c6d22. It adds a `JvmtiThreadState::set_thread_oop()` method, and checks the above observation with `state->get_thread_oop()`, and addresses the problem of `JvmtiThreadState::_thread_oop_h` incorrectly staying null for attached native thread.
> 
> I tested it with the 10000 virtual threads running Thread.sleep() example from https://openjdk.org/jeps/425, and  `make test TEST=jdk_loom`.

If my understand of [JavaThread::rebind_to_jvmti_thread_state_of](https://github.com/openjdk/jdk/blob/fe0ccdf5f8a5559a608d2e2cd2b6aecbe245c5ec/src/hotspot/share/runtime/javaThread.cpp#L1819) is correct, state->get_thread_oop() could be either `JavaThread::_threadObj` or `JavaThread::_jvmti_vthread`, ignoring the null case here.

The `state` could be null if the virtual thread is unmounted and the thread is null, or if a JvmtiThreadState has not been created for the thread.

Currently the `JvmtiThreadState::_thread_oop_h` is 'sealed' once a JvmtiThreadState is created. It can never be altered before a JvmtiThreadState is destroyed. Adding a `JvmtiThreadState:set_thread_oop()` changes that assumption. Let's wait for others who are more involved in this area to comment as well.

I just took a look of your https://github.com/openjdk/jdk/commit/00ace66c36243671a0fb1b673b3f9845460c6d22 change. 
I think it does not address the specific issue that we were seeing in our tests. At the time when `JvmtiThreadState::state_for_while_locked` is called for a JavaThread associated with a native thread being attached, the thread oop is being allocated. There is no non-null thread oop to be set into `JvmtiThreadState::_thread_oop_h`.

I'll address other comments later. Thanks, @caoman.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1391895645