On Mon, 13 Feb 2023 19:51:12 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 three
> additional commits since the last revision:
>
> - use different @test to run with -XX:-VMContinuations
> - more removals of @requires vm.continuations
> - change BasicVirtualThread_klass -> BaseVirtualThread_klass
I've left one comment. Otherwise test changes looks good to me.
test/hotspot/jtreg/serviceability/jvmti/vthread/BoundVThreadTest/BoundVThreadTest.java
line 27:
> 25: * @test
> 26: * @summary Verifies correct JVMTI behavior for BoundVirtualThreads.
> 27: * @compile --enable-preview -source ${jdk.version} BoundVThreadTest.java
You could use @enablePreview instead.
-------------
Marked as reviewed by lmesnik (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12512
