On Mon, 19 Oct 2026 05:00:20 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: > > Remove unused forward declaration Some comments. Please also run ad-hoc heap dump timings test to confirm we had not accidentally regressed dumping performance. See for example https://github.com/openjdk/jdk/pull/16545 src/hotspot/share/gc/shenandoah/shenandoahHeap.inline.hpp line 291: > 289: PLAB* gclab = ShenandoahThreadLocalData::gclab(thread); > 290: if (gclab == nullptr) { > 291: assert(!thread->is_Java_thread() && !is_gc_worker_thread(thread), So all this `is_gc_worker_thread` scaffolding is only to satisfy this particular assert, right? This walks all worker threads every time we ask. And we ask quite a bit from diagnostic workers, since they do not have PLABs. I think we can remove this assert here completely, and instead do the GCLAB/PLAB existence verification in verifier. There, we could also check that no _other_ threads _have_ a GCLAB/PLAB. This can be done in a separate RFE. src/hotspot/share/memory/heapInspection.cpp line 727: > 725: if (active_workers <= 1) { > 726: workers = nullptr; > 727: } Suggestion: // Use parallel workers if we have enough active workers and parallel heap iteration is supported. WorkerThreads* workers = nullptr; if (Universe::heap()->supports_parallel_object_iteration() && num_workers > 1) { const uint active_workers = DiagnosticWorkers::try_and_set_active_workers(num_workers); if (active_workers > 1) { workers = DiagnosticWorkers::workers(); } } src/hotspot/share/nmt/memMapPrinter.cpp line 232: > 230: } > 231: > 232: ThreadStackClosure diag_cl(from, to); Not sure why it is under `Universe::heap()` check here. The old code understandably does that since it performs later access to `Universe::heap()`. Sure, this works now since it serves as "bootstrap is completed" check. But maybe we can just do `DiagnosticWorkers::diagnostic_threads_do` without prior check, and will still be fine? src/hotspot/share/services/diagnosticWorkers.cpp line 38: > 36: assert(Thread::current()->is_VM_thread(), "Must be the VM thread"); > 37: assert_at_safepoint(); > 38: const uint max_workers = (uint)os::initial_active_processor_count(); Why `initial_*`, and not just `active_processor_count`? The diagnostic command likely comes way after the startup, so it stands to reason it should use the currently active cpu count. src/hotspot/share/services/diagnosticWorkers.cpp line 43: > 41: // Use release_store() because the error reporter can read _workers > while > 42: // the pool is being created. > 43: _workers.release_store(pool); Yeah, but concurrent calls to this method would leak one instance of `WorkerThreads`. You need to CAS the instance and free the instance that lost the installation race. ------------- PR Review: https://git.openjdk.org/jdk/pull/33218#pullrequestreview-5569155183 PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4314860423 PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4314897503 PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4314918728 PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4312005857 PR Review Comment: https://git.openjdk.org/jdk/pull/33218#discussion_r4314928814
