On Thu, 23 Mar 2023 18:08:47 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   address review comment: remove unneeded function
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java
>  line 148:
> 
>> 146: 
>> 147:     static private void setVirtualThreadsNotifyJvmtiMode(int iter, 
>> boolean enable) {
>> 148:         sleep(5);
> 
> Why is this needed?

It is needed to balance enabling/disabling notifyJvmti mode with the 
ThreadStart/VirtualThreadStart events.
Otherwise, many mode switches can be observed without any events which is not 
interesting.
We need to allow virtual threads to execute a little bit after a mode switch.

> test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java
>  line 171:
> 
>> 169:         sleep(20);
>> 170: 
>> 171:         for (int iter = 0; VirtualThreadStartedCount() < VTHREADS_CNT; 
>> iter++) {
> 
> The test seems to exit once all the threads are started. I would think you 
> would want it to run for a while after all the threads are started.

I'm not sure if it is really needed. 60 virtual threads are started.
Some of them are executed long enough before shutdown.
We can just increase the number of threads if necessary.

> test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/libToggleNotifyJvmtiTest.cpp
>  line 32:
> 
>> 30: 
>> 31: static jvmtiEnv *jvmti;
>> 32: static int vthread_started_cnt = 0;
> 
> Needs to be volatile?

Thanks. We use a RawMonitor for sync. But anyway I made it volatile now.

> test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/libToggleNotifyJvmtiTest.cpp
>  line 34:
> 
>> 32: static int vthread_started_cnt = 0;
>> 33: static jrawMonitorID agent_lock = NULL;
>> 34: static bool can_support_vt_enabled = false;
> 
> This variable doesn't seem to be needed. You set it `true` later on, but 
> never reference it.

Good catch. Removed.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147011556
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147013785
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147014957
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147016373

Reply via email to