RFR: 8306738: Select num workers for safepoint ParallelCleanupTask

[Aleksey Shipilev]

On Mon, 24 Apr 2023 16:16:37 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Select num workers for safepoint ParallelCleanupTask to be at most the number of parallel cleanup tasks `SAFEPOINT_CLEANUP_NUM_TASKS`
>> 
>> Unsure if `WorkerThreads::max_workers()` is more correct than `WorkerThreads::created_workers()` or `WorkerThreads::active_workers()`. 
>> 
>> Tested tier1-3 Oracle platforms.
>
> src/hotspot/share/runtime/safepoint.cpp line 607:
> 
>> 605:     // Parallel cleanup using GC provided thread pool.
>> 606:     const uint num_workers = MIN2<uint>(SAFEPOINT_CLEANUP_NUM_TASKS, cleanup_workers->max_workers());
>> 607:     cleanup_workers->run_task(&cleanup, num_workers);
> 
> If you go to the definition of `WorkerThreads::run_task(WorkerTask* task)`, then you'll notice it uses `_active_workers`. So we should be doing `MIN2(..., cleanup_workers->active_workers()` here if the current behavior is to be preserved better. I think active workers is correct when GC worker count is dynamic (see `UseDynamicNumberOfGCThreads`).

Now that I am looking at it, I wonder if we have a pre-existing performance issue here: what if active GC workers is very low for some GC-specific reason? We then run all safepoint cleanups, even for non-GC safepoints, using that small worker count? This might be the accidental reliance on the number of GC workers to match the static fraction of the number of CPUs allocated to the JVM, which is often true, but this would not hold exactly under `UseDynamicNumberOfGCThreads`.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13616#discussion_r1175534530