On Mon, 13 Nov 2023 23:21:56 GMT, Man Cao <m...@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
