On Tue, 12 Dec 2023 23:42:07 GMT, Leonid Mesnik <lmes...@openjdk.org> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: (1) rename notifyJvmti method; (2) add try-final statements to >> VirtualThread methods > > src/java.base/share/classes/java/lang/VirtualThread.java line 1164: > >> 1162: >> 1163: @IntrinsicCandidate >> 1164: private native void notifyJvmtiCriticalLock(boolean enter); > > The name is confusing to me, the CriticalLock looks like it is the section is > critical and might be taken by a single thread only. Or it's just unclear > what is critical here. > However, the purpose is to disable suspend > Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name > here? > or comment what critical means here. Okay, thanks. I like your name suggestion but let's check with Alan first. > test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java > line 30: > >> 28: * @requires vm.continuations >> 29: * @library /testlibrary >> 30: * @run main/othervm -Xint SuspendWithInterruptLock > > Doesn't it make sense to add a testcase without -Xint also? Just to give > stress testing with compilation. Thanks. I was also thinking about this. Will add a sub-test without -Xint. > test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java > line 36: > >> 34: >> 35: public class SuspendWithInterruptLock { >> 36: static boolean done; > > done is accessed from different threads, should be volatile. Good suggestion, thanks. > test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java > line 54: > >> 52: Thread.yield(); >> 53: } >> 54: done = true; > > I think it is better to use done to stop all threads and set it to true in > the main thread after some time. So you could be sure that the yielder hadn't > been completed before the suspender started. But it is just proposal. Thank you. Will consider this. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426638981 PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426635613 PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426636196 PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426637200