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

[Serguei Spitsyn]

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

>> Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address Serguei Spitsyn's comments/suggestions:
>>   - Remove the redundant thread->is_Java_thread() check from JvmtiSampledObjectAllocEventCollector::object_alloc_is_safe_to_sample().
>>   - Change the assert in JvmtiThreadState::state_for_while_locked to avoid #ifdef ASSERT.
>
> 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.

This also was caught by my eyes. :)
With the lines 99-101 in place the only case when `thread_oop` can be equal to `nullptr` is when `thread->threadObj() == nullptr`. My understanding is that can be for a detached thread only.
I would suggest to add an assert after the line 101:
  assert(thread_oop != nullptr, "sanity check");
Full testing with this assert should help to identify if it can be fired.
Then we can get rid of the check at the line 104.
The `JvmtiThreadState` constructor also allows for `thread_oop` to be `nullptr`.
Some cleanup will be needed to get rid of unneeded checks there as well.

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

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