On Thu, 16 Nov 2023 21:58:17 GMT, Man Cao <m...@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 > 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 @dholmes-ora Thank you for the reviews and discussions in this thread. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1832198014
