On Mon, 3 Apr 2023 23:11:49 GMT, Chris Plummer <cjplum...@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. Correct. Fixed. > 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". Fixed > 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. Added comment. Sleep does the job, I don't think it makes sense to overcomplicate the test > 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. expected_cnt > 1 means there are references to 2 objects of the class or 2 references to the same object. I don't see how this would improve test coverage. 1 (or 0) reference to each object helps to keep the test simple > 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. if count is > 1 the will fail later verifying the value I added "ERROR" logging ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156603534 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156603638 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156605011 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156573479 PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1156603364
