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 > [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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/16642#issuecomment-1815667615
