On Fri, 29 Sep 2023 02:20:10 GMT, Alex Menkov <amen...@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
