On Thu, 23 Mar 2023 05:54:29 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> The fix is to enable virtual threads support for late binding JVMTI agents.
>> The fix includes:
>> - New function `JvmtiEnvBase::enable_virtual_threads_notify_jvmti()` which
>> does enabling JVMTI VTMS transition notifications in case of agent loaded
>> into running VM. This function executes a VM operation counting VTMS
>> transition bits in all `JavaThread`'s to correctly set the static counter
>> `_VTMS_transition_count` needed for VTMS transition protocol.
>> - New function `JvmtiEnvBase::disable_virtual_threads_notify_jvmti()` which
>> is needed for testing. It is used by the `WhiteBox` API.
>> - New WhiteBox function `WB_SetVirtualThreadsNotifyJvmtiMode(JNIEnv* env,
>> jobject wb, jboolean enable)` needed for testing of this update.
>> - New regression test: `serviceability/jvmti/vthread/ToggleNotifyJvmtiTest`
>>
>> Testing:
>> - New test: `serviceability/jvmti/vthread/ToggleNotifyJvmtiTest`
>> - The originally failed tests are expected to pass now:
>> `runtime/vthread/RedefineClass.java`
>> `runtime/vthread/TestObjectAllocationSampleEvent.java`
>> - In progress: Run the tiers 1-6 to make sure there are no regression.
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> address review comment: remove unneeded function
src/hotspot/share/prims/jvmtiThreadState.hpp line 101:
> 99: static void set_VTMS_notify_jvmti_events(bool val) {
> _VTMS_notify_jvmti_events = val; }
> 100:
> 101: static void set_VTMS_transition_count(bool val) {
> _VTMS_transition_count = val; }
Why set the count if it is never going to be used?
test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java
line 38:
> 36: */
> 37:
> 38: //import compiler.whitebox.CompilerWhiteBoxTest;
Remove
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?
test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java
line 72:
> 70: if (i > n) {
> 71: i = 0;
> 72: n = n - 1;
n--
test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java
line 74:
> 72: n = n - 1;
> 73: }
> 74: i = i + 1;
i++
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?
test/hotspot/jtreg/serviceability/jvmti/vthread/ToggleNotifyJvmtiTest/ToggleNotifyJvmtiTest.java
line 157:
> 155:
> 156: if (args.length > 0 && args[0].equals("attach")) { // agent
> loaded into running VM case
> 157: String arg = args.length == 2 ? args[1] : "";
I don't see any args being passed in other than "attach"? What might `arg` be
set to?
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`?
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.
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?
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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146578471
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146594032
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146608558
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146598256
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146598429
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146625993
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146620881
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146624531
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146629460
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146630461
PR Review Comment: https://git.openjdk.org/jdk/pull/13133#discussion_r1146633746
