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

[Chris Plummer]

On Fri, 24 Mar 2023 00:23:19 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java line 68:
>> 
>>> 66:             if (n <= 0) {
>>> 67:                 n = 1000;
>>> 68:                 ToggleNotifyJvmtiTest.sleep(1);
>> 
>> It looks like you do this short sleep 1 out of every 1,000,000 iterations. Can you explain why?
>
> It is for yielding. Do you think we need this with a bigger frequency?

I guess the question then is why the need to yield. It just seems a bit odd that I thought the main point of this loop was to keep busy calling `breakpointCheck()`, and I don't see how doing a yield 1 out of every 1,000,000 iterations relates to that.

>> test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java line 72:
>> 
>>> 70:             if (i > n) {
>>> 71:                 i = 0;
>>> 72:                 n = n - 1;
>> 
>> n--
>
> This code was copied originally from the vmTestbase to SuspendThread* tests and some other tests.
> I can do all suggested simplifications but not sure if it is really necessary.
> It does not matter what exactly the method does because it just simulates some thread activity.
> Would it better to keep copy/pasted methods the same?

ok

>> 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.

Shouldn't that be the caller's responsibility? Including a comment would be helpful.

>> test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java line 161:
>> 
>>> 159:             vm.loadAgentLibrary(AGENT_LIB, arg);
>>> 160:         } else {
>>> 161:             System.loadLibrary(AGENT_LIB);
>> 
>> Why is this needed? Isn't the library already loaded due to it being specified by `-agentlib`?
>
> Good question. We almost always do it in the JVMTI tests including `serviceability/jvmti/vthread` and `vmTestbase/nsk/jvmti` tests. Examples are 22 `serviceability/jvmti/vthread` tests.

Are you saying it's not needed, but you included it to be consistent with other tests?

>> 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.

ok

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147966083
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147966677
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147969519
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147969967
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1147970566