On Tue, 12 Dec 2023 23:42:07 GMT, Leonid Mesnik <[email protected]> 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
