RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads [v3]

[Serguei Spitsyn]

On Fri, 29 Sep 2023 02:20:10 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

>> This is subtask of JDK-8299426: Heap dump does not contain virtual Thread stack references
>> The change:
>> - reorganize thread-related code/prepare it to use for unmounted vthreads:
>>   - new ThreadDumper class caches stack frames, thread serial num, frame serial number (trace serial number is calculated from thread serial);
>>     ThreadDumper objects for all platform/carrier and mounted virtual threads are cached instead of ThreadStackTrace objects (they are created during HPROF_FRAME/HPROF_TRACE dumping, used lated for writing HPROF_GC_ROOT_THREAD_OBJ/HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL subrecords);
>>   - new helper class JavaStackRefDumper to dump references from threadf stack;
>> - separate track traces for mounted virtual threads:
>>   - separate HPROF_FRAME/HPROF_TRACE records for mounted vthreads and carrier threads;
>>   - separate HPROF_GC_ROOT_THREAD_OBJ/HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL subrecords;
>> - updated hprof parser test lib to collect data about threads (HPROF_GC_ROOT_THREAD_OBJ subrecords) and corresponding stack traces and stack references.
>> 
>> Testing - tier1-tier3, new test
>> 
>> Output of the test for VtreadInHeapDumpTarg$VthreadMounted thread
>> without the fix:
>> `thread 0x8101be90, 16 frames
>>   - [0] VtreadInHeapDumpTarg$VthreadMounted.run()V (VtreadInHeapDump.java:127)
>>       Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted
>>       Java Local Reference: VtreadInHeapDumpTarg$VThreadMountedReferenced
>>   - [1] java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V (Thread.java:1583)
>>       Java Local Reference: java.lang.VirtualThread
>>       Java Local Reference: java.lang.Class
>>       Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted
>>   - [2] java.lang.VirtualThread.run(Ljava/lang/Runnable;)V (VirtualThread.java:309)
>>       Java Local Reference: java.lang.VirtualThread
>>       Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted
>>       Java Local Reference: java.lang.Class
>>   - [3] java.lang.VirtualThread$VThreadContinuation$1.run()V (VirtualThread.java:190)
>>       Java Local Reference: java.lang.VirtualThread$VThreadContinuation$1
>>   - [4] jdk.internal.vm.Continuation.enter0()V (Continuation.java:320)
>>       Java Local Reference: java.lang.VirtualThread$VThreadContinuation
>>   - [5] jdk.internal.vm.Continuation.enter(Ljdk/internal/vm/Continuation;Z)V (Continuation.java:312)
>>       Java Local Reference: java.lang.VirtualThread$VThreadContinuation
>>   - [6] jd...
>
> Alex Menkov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   misspell getRefererId for consistency

The fix looks pretty good to me.
I've posted several nits though.

Marked as reviewed by sspitsyn (Reviewer).

src/hotspot/share/services/heapDumper.cpp line 1494:

> 1492: class ThreadDumper : public CHeapObj<mtInternal> {
> 1493: public:
> 1494:   enum class ThreadType {Platform, MountedVirtual, UnmountedVirtual};

Please, add space after `{` and before `}`.

src/hotspot/share/services/heapDumper.cpp line 2034:

> 2032: 
> 2033:   // HPROF_GC_ROOT_THREAD_OBJ records for platform and mounted virtual threads
> 2034:   void dump_platform_threads();

I'd suggest to rename it to `dump_threads()` as the suffix `_platform` is confusing as the comment says it is also dumping virtual threads.

src/hotspot/share/services/heapDumper.cpp line 2045:

> 2043: 
> 2044:   // HPROF_TRACE and HPROF_FRAME records for platform and mounted virtual threads
> 2045:   void dump_platform_stack_traces();

This name is a little confusing. I's suggest to keep `name dump_stack_traces()`.

test/hotspot/jtreg/serviceability/jvmti/vthread/HeapDump/VThreadInHeapDump.java line 241:

> 239:             test(snapshot, VThreadInHeapDumpTarg.VThreadMountedReferenced.class);
> 240:             test(snapshot, VThreadInHeapDumpTarg.PThreadReferenced.class);
> 241:             //test(snapshot, VThreadInHeapDumpTarg.VThreadUnmountedReferenced.class);

It'd be nice to add a comment explaining why this line was commented out.

-------------

Marked as reviewed by sspitsyn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/15869#pullrequestreview-1662624022
PR Review: https://git.openjdk.org/jdk/pull/15869#pullrequestreview-1662671469
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1349223802
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1349206481
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1349204094
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1349233417