On Wed, 15 Feb 2023 05:04:21 GMT, Serguei Spitsyn <[email protected]> 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
