RFR: 8306738: Select num workers for safepoint ParallelCleanupTask

[Thomas Schatzl]

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

>> 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`.

It is a "known" issue that if the particular task does not select its number of threads properly before executing the task (or not at all like this task) that there may be perf issues.

If this is what you want I would suggest that `active_workers()` as limit preserves the current behavior best. Otherwise the task should calculate and set its optimal number of tasks itself as it does in this change.

Not sure if setting it to `SAFEPOINT_CLEANUP_NUM_TASKS` is a good idea if some/most of this work is trivial/empty but it's probably a good first guess.

The only GC that does not seem to set safepoint_workers to something non-default seems to be Shenandoah (the stw collectors use the "main" workerthreads they also use for evacuation, so this should be maxed out mostly, and ZGC explicitly sets them to max_workers at initialization). (via code inspection, so might be wrong)

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

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