RFR: 8299414: JVMTI FollowReferences should support references from VirtualThread stack

[Alex Menkov]

On Mon, 3 Apr 2023 23:11:49 GMT, Chris Plummer <cjplummer at 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