On Wed, 15 Feb 2023 01:22:33 GMT, Patricio Chilano Mateo
<[email protected]> 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/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.
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.
-------------
PR: https://git.openjdk.org/jdk/pull/12512
