On Fri, 17 Nov 2023 02:51:03 GMT, Jiangli Zhou <jian...@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
