On Fri, 10 Feb 2023 14:50:36 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, > PopFrame, ForceEarlyReturnXXX) 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 I see that JVM_GetAllThreads will now returned the filtered list (as an array). That looks okay. There's another part to this in the ThreadMXBean implementation where the filtering is done in the Java implementation. We can look at that in a follow-up issue, to avoid adding more to this PR. On tests, there are a few other tests that can be changed to drop `@requires vm.continuations`, e.g. the jshell tests that run with --enable-preview should no longer need the `@requires`. There are also a few JDI and at least one AppCDS test. A search will find them. The tests re-running with -XX:-VMContinuations is fine but it does mean they will run twice on the ports without an implementation. The test runtime/jni/IsVirtualThread/IsVirtualThread.java is an example where it only runs once on these ports, at the cost of additional `@test` tag. ------------- PR: https://git.openjdk.org/jdk/pull/12512
