On Tue, 14 Nov 2023 20:57:09 GMT, Jiangli Zhou <jian...@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 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? ------------- PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1815379890
