RFR: 8300575: JVMTI support when using alternative virtual thread implementation [v3]

[Patricio Chilano Mateo]

On Wed, 15 Feb 2023 05:04:21 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

> Patricio, The fix looks pretty solid to me. I've already posted one comment and will post a couple of formatting nits. I agree with Alan, it'd be nice to get rid of `is_bound_vthread` if possible. I will make one more pass after your update. Also, I'd address the JVMTI spec issue (mentioned by Alan) in a separate CR that needs to go with a CSR. I'll file an RFE or spec bug on it. Thanks, Serguei
>
Thanks for looking at this Serguei.

> src/hotspot/share/prims/jvmtiEnv.cpp line 1045:
> 
>> 1043:             JvmtiEnvBase::is_vthread_alive(vt_oop) &&
>> 1044:             !JvmtiVTSuspender::is_vthread_suspended(vt_oop)) ||
>> 1045:             java_thread->is_bound_vthread()) &&
> 
> This condition does not look correct.
> I'd expect it to be:
>  ```
>          ((java_lang_VirtualThread::is_instance(vt_oop) &&
>            JvmtiEnvBase::is_vthread_alive(vt_oop)) ||
>           java_thread->is_bound_vthread()) &&
>           !JvmtiVTSuspender::is_vthread_suspended(vt_oop) &&
> 
> It is important to check for `!JvmtiVTSuspender::is_vthread_suspended(vt_oop)` for `bound_vthread` as well.
> The same issue is in the `JvmtiEnv::ResumeAllVirtualThreads`.

The thing is that JvmtiVTSuspender::is_vthread_suspended() checks _suspended_list/not_suspended_list which are not set with single suspend/resume functions(SuspendThread/SuspendThreadList/ResumeThread/ResumeThreadList) on a BoundVirtualThread. For single suspend/resume on a BoundVirtualThread we go through the same path as for platform threads. In any case, the suspend_thread() call will just return JVMTI_ERROR_THREAD_SUSPENDED if a thread is already suspended, but if you want to avoid the call altogether I can add the equivalent check which would be !java_thread->is_suspended().

Now, I realized that although single-suspends will not change _suspended_list/not_suspended_list for a BoundVirtualThread, SuspendAllVirtualThreads/ResumeAllVirtualThreads can if a BoundVirtualThread is part of the exception list. In the SuspendAllVirtualThreads case for example, when restoring the resume state, the call to JvmtiVTSuspender::is_vthread_suspended() would return true (we are already in SR_all mode and the BoundVirtualThread will not be in _not_suspended_list), meaning the BoundVirtualThread will be added to the not_suspended_list. 
So we have to decide if we want to keep track of BoundVirtualThreads on these lists or not. Given that I see JvmtiVTSuspender::is_vthread_suspended() is always called on a VirtualThread the easiest thing would be to not track them, and just add a check in SuspendAllVirtualThreads/ResumeAllVirtualThreads where the elist is populated so only VirtualThreads are added. What do you think?

> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1587:
> 
>> 1585:   if (!is_passive_cthread) {
>> 1586:     assert(thread_h() != nullptr, "sanity check");
>> 1587:     assert(single_suspend || thread_h()->is_a(vmClasses::BaseVirtualThread_klass()), "SuspendAllVirtualThreads should never suspend non-virtual threads");
> 
> Nit: It is better to split this line by placing assert message on a separate line.

Fixed.

> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1648:
> 
>> 1646:   if (!is_passive_cthread) {
>> 1647:     assert(thread_h() != nullptr, "sanity check");
>> 1648:     assert(single_resume || thread_h()->is_a(vmClasses::BaseVirtualThread_klass()), "ResumeAllVirtualThreads should never resume non-virtual threads");
> 
> Nit: It is better to split this line by placing assert message on a separate line.

Fixed.

-------------

PR: https://git.openjdk.org/jdk/pull/12512