RFR: JDK-8306441: Two phase segmented heap dump [v25]

[Yi Yang]

On Thu, 3 Aug 2023 13:24:19 GMT, Kevin Walls <kevinw at openjdk.org> wrote:

> Also the variable num_requested_dump_thread is not needed? It's just a copy of _num_dumper_threads which we don't change.

This is needed because _num_dumper_threads will change later, the requested dump thread may not equal to actual _num_dumper_threads. `is_parallel_dump()` checks _num_dump_thread, so we cannot use suggested code.

This involves code style favor, to avoid futile effort, do you think the below code is acceptable?

--- a/src/hotspot/share/services/heapDumper.cpp
+++ b/src/hotspot/share/services/heapDumper.cpp
@@ -1724,15 +1724,8 @@ class VM_HeapDumper : public VM_GC_Operation, public WorkerTask {
   }
   int dump_seq()           { return _dump_seq; }
   bool is_parallel_dump()  { return _num_dumper_threads > 1; }
-  bool can_parallel_dump() {
-    const char* base_path = writer()->get_file_path();
-    assert(base_path != nullptr, "sanity check");
-    if ((strlen(base_path) + 7/*.p\d\d\d\d\0*/) >= JVM_MAXPATHLEN) {
-      // no extra path room for separate heap dump files
-      return false;
-    }
-    return true;
-  }
+  bool can_parallel_dump(uint num_active_workers);
+
   VMOp_Type type() const { return VMOp_HeapDumper; }
   virtual bool doit_prologue();
   void doit();
@@ -1923,6 +1916,33 @@ bool VM_HeapDumper::doit_prologue() {
   return VM_GC_Operation::doit_prologue();
 }
 
+bool VM_HeapDumper::can_parallel_dump(uint num_active_workers) {
+  assert(base_path != nullptr, "sanity check");
+  bool has_extra_path_room = true;
+  const char* base_path = writer()->get_file_path();
+  if ((strlen(base_path) + 7/*.p\d\d\d\d\0*/) >= JVM_MAXPATHLEN) {
+    // no extra path room for separate heap dump files
+    has_extra_path_room = false;
+  }
+
+  bool can_parallel = true;
+  uint num_requested_dump_thread = _num_dumper_threads;
+  if (num_active_workers <= 1 ||         // serial gc?
+      num_requested_dump_thread <= 1 ||  // request serial dump?
+     !has_extra_path_room) {             // has extra path room for segmented dump files?
+    _num_dumper_threads = 1;
+    can_parallel = false;
+  } else {
+    _num_dumper_threads = clamp(num_requested_dump_thread, 2U, num_active_workers);
+    can_parallel = true;
+  }
+
+  log_info(heapdump)("Requested dump threads %u, active dump threads %u, "
+                     "actual dump threads %u, parallelism %s",
+                     num_requested_dump_thread, num_active_workers,
+                     _num_dumper_threads, can_parallel ? "true" : "false");
+  return can_parallel;
+}
 
 // The VM operation that dumps the heap. The dump consists of the following
 // records:
@@ -1970,20 +1990,10 @@ void VM_HeapDumper::doit() {
 
   WorkerThreads* workers = ch->safepoint_workers();
   uint num_active_workers = workers != nullptr ? workers->active_workers() : 0;
-  uint num_requested_dump_thread = _num_dumper_threads;
-  bool can_parallel = can_parallel_dump();
-  log_info(heapdump)("Requested dump threads %u, active dump threads %u, " "parallelism %s",
-                      num_requested_dump_thread, num_active_workers, can_parallel ? "true" : "false");
 
-  if (num_active_workers <= 1 ||         // serial gc?
-      num_requested_dump_thread <= 1 ||  // request serial dump?
-     !can_parallel) {                    // can not dump in parallel?
-    // Use serial dump, set dumper threads and writer threads number to 1.
-    _num_dumper_threads = 1;
+  if (can_parallel_dump(num_active_workers)) {
     work(VMDumperWorkerId);
   } else {
-    // Use parallel dump otherwise
-    _num_dumper_threads = clamp(num_requested_dump_thread, 2U, num_active_workers);
     uint heap_only_dumper_threads = _num_dumper_threads - 1 /* VMDumper thread */;
     _dumper_controller = new (std::nothrow) DumperController(heap_only_dumper_threads);
     ParallelObjectIterator poi(_num_dumper_threads);


> out.shouldContain(......then we get the output included in the log if it fails, so we should see what went wrong.

Acknowledge

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

PR Comment: https://git.openjdk.org/jdk/pull/13667#issuecomment-1664913578