On Mon, 13 Nov 2023 23:33:50 GMT, Man Cao <m...@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
