On Wed, 15 Feb 2023 01:22:33 GMT, Patricio Chilano Mateo
<pchilanom...@openjdk.org> wrote:
>> Please review the following change. Some of the JVMTI functions had to be
>> modified since they are not supported by the specs on virtual threads
>> (StopThread, RunAgentThread, GetCurrentThreadCpuTime, GetThreadCpuTime) or
>> shouldn't include virtual threads in the results (GetAllThreads,
>> GetAllStackTraces, GetThreadGroupChildren). Others were modified because
>> they should work on virtual thread but they weren't
>> (SuspendAllVirtualThreads/ResumeAllVirtualThreads). Also support for
>> VirtualThreadStart/VirtualThreadEnd events was added.
>>
>> There were a total of 71 tests under serviceability/jvmti/ that had the
>> vm.continuations requirement. Of those, 24 where under
>> serviceability/jvmti/vthread/. I was able to remove that requirement from 50
>> tests. Most of those tests were already passing for the alternative vthread
>> implementation just by removing the check in jni_GetEnv for VMContinuations.
>> From the other 21 tests, 20 still fail with the changes either because they
>> actually test continuations or because of different assumptions in the tests
>> that don't hold true for the alternative vthread implementation. So for
>> those I left the vm.continuations requirement untouched (from those 20 tests
>> there are actually 4 tests from the thread/GetStackTrace/ family that are
>> passing because of a bug in the test, but I can fix that in another RFE).
>> The other remaining test is vthread/GetSetLocalTest/GetSetLocalTest.java
>> which I see is problemlisted.
>>
>> Regarding variable _is_bound_vthread, although it's handy as a cache to
>> avoid repeating the check, I mainly added it to avoid transitioning for
>> GetCurrentThreadCpuTime (we are native and cannot access oops).
>>
>> I added new test BoundVThreadTest.java which just checks for the
>> unsupported/supported behavior mentioned previously. For some extra basic
>> coverage I also added a new run with -XX:-VMContinuations on tests inside
>> the serviceability/jvmti/vthread/ directory that don't require
>> vm.continuations anymore. I could also add that for all the other tests in
>> serviceability/jvmti/ for which I removed the vm.continuations requirement.
>>
>> I run the patch through mach5 tiers 1-6 plus some extra local runs on the
>> relevant tests.
>>
>> Thanks,
>> Patricio
>
> Patricio Chilano Mateo has updated the pull request incrementally with one
> additional commit since the last revision:
>
> use @enablePreview instead
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`.
-------------
PR: https://git.openjdk.org/jdk/pull/12512
