On Thu, 30 Mar 2023 22:58:12 GMT, Alex Menkov <amen...@openjdk.org> wrote:
> The fix updates JVMTI FollowReferences implementation to report references > from virtual threads: > - added heap scanning to report unmounted vthreads; > - stacks of mounted vthreads are splitted into 2 parts (vittual thread stack > and carrier thread stack), references are reported with correct thread > id/class and object tags/frame depth; > - common code to handle stack frames are moved into separate class; test/hotspot/jtreg/serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java line 54: > 52: } > 53: } > 54: await(dumpedLatch); await() seems unnecessary given the use the !timeToStop flag. test/hotspot/jtreg/serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java line 83: > 81: System.out.println(referenced.getClass()); > 82: }); > 83: vthreadEnded.join(); Add comment that says something like "Make sure this vthread has exited so we can test that it no longer holds any stack references". test/hotspot/jtreg/serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java line 85: > 83: vthreadEnded.join(); > 84: > 85: Thread.sleep(2000); // wait for reference and unmount I think what you mean is you need to wait until the threads have made enough progress to create the references, and then you need to wait until they have had a chance to amount due to the await() call. This should be made more clear in the comments. BTW, you could choose to get JVMTI VIRTUAL_THREAD_UNMOUNT events, and instead block here until you get them all, but doing a sleep is a lot easier. test/hotspot/jtreg/serviceability/jvmti/vthread/FollowReferences/VThreadStackRefTest.java line 94: > 92: // expected to be unreported as stack local > 93: new TestCase(VThreadUnmountedEnded.class, 0, 0) > 94: }; I think it would be useful the have a test case which has expected_cnt > 1. test/hotspot/jtreg/serviceability/jvmti/vthread/FollowReferences/libVThreadStackRefTest.cpp line 72: > 70: jvmtiHeapReferenceInfoStackLocal *stackInfo = > (jvmtiHeapReferenceInfoStackLocal *)reference_info; > 71: refCounters.count[index]++; > 72: refCounters.threadId[index] = stackInfo->thread_id; If `count` is >1 at this point, can this line be an assert? I assume the threadId should never change for any given index once it is set. ------------- PR Review: https://git.openjdk.org/jdk/pull/13254#pullrequestreview-1369892549 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156534139 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156534779 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156538712 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156540510 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156520354
