RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v5]

[Serguei Spitsyn]

On Thu, 23 Mar 2023 18:08:47 GMT, Chris Plummer <cjplummer at 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