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

[Man Cao]

On Mon, 13 Nov 2023 21:52:22 GMT, Jiangli Zhou <jiangli 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.

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