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

[Jiangli Zhou]

On Fri, 17 Nov 2023 02:51:03 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

>> 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.

> > > @jianglizhou - I fixed a typo in the bug's synopsis line. Change this PR's title: s/is create/is created/
> > > Thanks, @dcubed-ojdk!
> 
> Now, the PR title needs to be fixed accordingly.

Done, thanks for the reminder!

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

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