On Thu, 15 Oct 2026 21:29:19 GMT, Patrick Fontanilla <[email protected]> wrote:
>> ### Summary >> For Shenandoah and ZGC, safepoint workers were created at startup and only >> used for heap dump and heap inspection. If heap dump or heap inspection >> never runs, these threads consume memory without doing any work. For G1 and >> Parallel, GC pauses and non-GC safepoints share the GC worker pool, so heap >> dump and heap inspection don't need a separate worker pool. The downside is >> that dump and inspection work contributes to GC CPU time, which can affect >> how G1 sizes its heap. >> >> ### Changes >> Safepoint workers are replaced with a diagnostic worker pool used by heap >> dump and heap inspection. The diagnostic worker pool is created on the first >> heap dump or heap inspection that requests more than one thread, if the >> collector supports parallel heap iteration and >> `os::initial_active_processor_count()` > 1. The pool holds at most >> `os::initial_active_processor_count()` workers, and workers stay alive until >> the VM exits. >> >> G1 and Parallel no longer lend their GC worker pool. ZGC's `ZRuntimeWorkers` >> is deleted because it's no longer used. For Shenandoah, diagnostic workers >> have no GCLAB and fall back to shared allocation when they evacuate objects. >> >> ### Testing all passed >> - linux-x86_64-server-fastdebug: `hotspot:tier1_gc`, >> `hotspot:tier1_serviceability`, `gc/shenandoah` >> - GHA >> - Internal CI pipeline >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Patrick Fontanilla has updated the pull request incrementally with one > additional commit since the last revision: > > Add comment to declaration of _workers src/hotspot/share/gc/shared/collectedHeap.hpp line 480: > 478: // Returns true if the collector implements > parallel_object_iterator(), so > 479: // heap dump and heap inspection may iterate the heap with multiple > worker threads. > 480: virtual bool supports_parallel_heap_iteration() const { return false; } I have two requests for this function: 1) That it moves up to above the `parallel_object_iterator`function 2) That it is renamed to `supports_parallel_object_iteration` to make it a tiny bit clearer that these two functions go hand-in-hand. src/hotspot/share/gc/shared/gcVMOperations.cpp line 211: > 209: } else { > 210: inspect.heap_inspection(_out, 1); > 211: } To me this seems like we have two places where we place the logic around how many GC threads to use: One here, and one directly inside the `heap_inspection` function. What do you think about moving all of this into the heap_inspection function? src/hotspot/share/services/diagnosticWorkers.hpp line 60: > 58: }; > 59: > 60: #endif // SHARE_GC_SHARED_DIAGNOSTICWORKERS_HPP Suggestion: #endif // SHARE_SERVICES_DIAGNOSTICWORKERS_HPP src/hotspot/share/services/heapDumper.cpp line 2666: > 2664: } > 2665: > 2666: prepare_parallel_dump(workers); The `prepare_parallel_dump` function looks at "active workers" and adjusts `_num_dumper_theads` accordingly. It seems like there's some kind of circularity going on there. Could you take a look at that and make sure that the code is still necessary and doing the right thing? Maybe the setup of active workers could be moved into prepare_parallel_dump? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4291913143 PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4292026239 PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4291980291 PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4291976157
