RFR: 8299426: Heap dump does not contain virtual Thread stack references

[Chris Plummer]

On Tue, 14 Nov 2023 21:54:06 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

> The change impelements dumping of unmounted virtual threads data (stack traces and stack references).
> Unmounted vthreads can be detected only by iterating over the heap, but hprof stack trace records (HPROF_FRAME/HPROF_TRACE) should be written before HPROF_HEAP_DUMP/HPROF_HEAP_DUMP_SEGMENT.
> HeapDumper supports segment dump (parallel dump to separate files with subsequent file merge outside of safepoint), the fix switches HeapDumper  to always use segment dump: 1st segment contains only non-heap data, other segments are used for dumping heap objects. For serial dumping single-threaded dumping is performed, but 2 segments are created anyway.
> When HeapObjectDumper detects unmounted virtual thread, it writes HPROF_FRAME/HPROF_TRACE records to the 1st segment ("global writer"), and writes thread object (HPROF_GC_ROOT_JAVA_FRAME) and stack references (HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL) to the HeapObjectDumper segment.
> As parallel dumpers may write HPROF_FRAME/HPROF_TRACE concurrently and VMDumper needs to write non-heap data before heap object dumpers can write virtual threads data, writing to global writer is protected with DumperController::_global_writer_lock.
> 
> Testing: run tests which perform heap dump (in different scenarios):
>  - test/hotspot/jtreg/serviceability
>  - test/hotspot/jtreg/runtime/ErrorHandling
>  - test/hotspot/jtreg/gc/epsilon
>  - test/jdk/sun/tools/jhsdb

I think you are dumping all virtual threads found in the heap, even if they are unreachable. Although this would be true for platform threads also, there is a high likelihood of a lot of unreachable virtual threads in the heap, but not so much for platform threads.

Also, have you tested scalability? Not just a large number of live virtual threads (like a million), but also combined with a large number of unreachable virtual threads.

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

> 1884: };
> 1885: 
> 1886: // Support class using when iterating over the heap.

Suggestion:

// Support class used when iterating over the heap.

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

> 2024:     size_t buf_size = strlen(base_path)
> 2025:                     + 2                 // ".p"
> 2026:                     + 1 + (seq / 10)    // number

Is this suppose to be the number of characters the sequence number will take up in the path? If so, that's not the correct calculation. For example, 35 ends up as 1 + 3 == 4, but it should be 2.

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

> 2132:   for (int i = 0; i < _dump_seq; i++) {
> 2133:     ResourceMark rm;
> 2134:     const char* path = get_writer_path(_path, i);

It's not clear to me why you've chosen to move from a JVM_MAXPATHLEN stack buffer to a dynamically allocated one.

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

PR Review: https://git.openjdk.org/jdk/pull/16665#pullrequestreview-1756134402
PR Review Comment: https://git.openjdk.org/jdk/pull/16665#discussion_r1409816743
PR Review Comment: https://git.openjdk.org/jdk/pull/16665#discussion_r1409831151
PR Review Comment: https://git.openjdk.org/jdk/pull/16665#discussion_r1409835597