On Fri, 24 Mar 2023 00:23:19 GMT, Serguei Spitsyn <[email protected]> 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
