On Tue, 14 Nov 2023 21:54:06 GMT, Alex Menkov <amen...@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
