On Mon, 13 Nov 2023 21:52:22 GMT, Jiangli Zhou <jian...@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. Changes requested by manc (Committer). src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 94: > 92: // The state->get_thread_oop() may be null if the state is created > during > 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. I think it is important to maintain `JvmtiThreadState::_thread_oop_h` correctly for the attached native thread. In the existing logic, with and without the proposed change, `JvmtiThreadState::_thread_oop_h` could stay null for an attached native thread, which seems wrong. It may be OK since `JvmtiThreadState::_thread_oop_h` is only used by support for virtual threads. It is unlikely that an attached native thread becomes a carrier for a virtual thread. However, it is probably still desirable to update `JvmtiThreadState::_thread_oop_h` to the correct java.lang.Thread oop. 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`. src/hotspot/share/prims/jvmtiThreadState.inline.hpp line 98: > 96: state->get_thread_oop() != thread_oop)) { > 97: // Check if java_lang_Thread already has a link to the > JvmtiThreadState. > 98: if (thread_oop != nullptr) { // thread_oop can be null during early > VMStart. This comment is another case of `state->get_thread_oop()` being null. We should merge this comment with the new comment about attaching native thread. ------------- PR Review: https://git.openjdk.org/jdk/pull/16642#pullrequestreview-1728423451 PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1391795730 PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1391805673 PR Review Comment: https://git.openjdk.org/jdk/pull/16642#discussion_r1391813214
