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

[Jiangli Zhou]

On Tue, 14 Nov 2023 20:57:09 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.
>
> Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Don't try to setup_jvmti_thread_state for obj allocation sampling if the current thread is attaching from native and is allocating the thread oop. That's to make sure we don't create a 'partial' JvmtiThreadState.

> Thanks. The latest change to `JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample()` looks OK to me. Skipping a few allocations for JVMTI allocation sampler is better than resulting in a problematic `JvmtiThreadState` instance.
> 
> My main question is if we can now change `if (state == nullptr || state->get_thread_oop() != thread_oop) ` to `if (state == nullptr)` in `JvmtiThreadState::state_for_while_locked()`. I suspect we would never run into a case of `state != nullptr && state->get_thread_oop() != thread_oop` with the latest change, even with virtual threads. This is backed up by testing with [00ace66](https://github.com/openjdk/jdk/commit/00ace66c36243671a0fb1b673b3f9845460c6d22) not triggering any failure.
> 
> If we run into such as a case, it could still be problematic as `JvmtiThreadState::state_for_while_locked()` would allocate a new `JvmtiThreadState` instance pointing to the same JavaThread, and it does not delete the existing instance.
> 
> Could anyone with deep knowledge on JvmtiThreadState and virtual threads provide some feedback on this change and https://bugs.openjdk.org/browse/JDK-8319935? @AlanBateman, do you know who would be the best reviewer for this?

@caoman and I discussed about his suggestion on changing `if (state == nullptr || state->get_thread_oop() != thread_oop)` check in person today. Since it may affect vthread, my main concern is that our current testing may not cover that sufficiently. The suggestion could be worked by a separate enhancement bug.

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

PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1815667615