On Thu, 3 Aug 2023 13:24:19 GMT, Kevin Walls <kev...@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
