RFR: 8227060: Optimize safepoint cleanup subtask order [v2]

[Kim Barrett]

On Fri, 15 Jul 2022 20:15:54 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Most of the analysis in the CR is for code that's removed, but I found one safepoint cleanup task that's unused.  Also the dictionary resizing and symbol/string table rehashing, while rare, could take a long time so I moved them sooner in the list.
>> Tested with tier1-3.
>
> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Kim's improvement ideas.

A bit of history here. It seems that some of the point of the original CR has
disappeared. There used to be a use of parallel_java_threads_do at the start
(from JDK-8180932), with the CR suggesting moving that to the end. That
disappeared with JDK-8246476. So some of the point of the CR has gone away.
But ordering the remaining tasks from expensive to cheap is still sensible,
and the new order mostly [*] looks plausible.

A different (pre-existing) problem is that the use of the workers isn't great.
The amount of parallelism is just the workgang's current `active_workers()`,
with no regard to how much parallelism we have. Presently the maximum useful
amount of parallelism is the number of subtasks, so 6 (which might easily and
likely be reduced with some pre-checks). So we're going to apply
`active_workers` threads (whatever that happens to be at the moment) to a task
which can use only a relatively small and fixed [*] number of threads.
Improving the use of the workgang should be a separate RFE.

[*] JDK-8253180 later (after JDK-8246476) introduced the serial threads_do to
set GC watermarks. Digging into it a bit, that doesn't look obviously
lightweight; I wonder if it should be (should have been) parallelized (and
placed at the end of the work). But that's a separate RFE.

src/hotspot/share/runtime/safepoint.cpp line 522:

> 520: private:
> 521:   SubTasksDone _subtasks;
> 522:   uint _num_workers;

[pre-existing] `_num_workers` seems to no longer be used.

src/hotspot/share/runtime/safepoint.cpp line 541:

> 539:   };
> 540: 
> 541:   class SafepointCleanupThreadClosure : public ThreadClosure {

The new name doesn't seem to have much more to do with what it does than did the old name.  The class definition could be moved to the single point of use and just called "Closure" to avoid needing to come up with a good name :)

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

PR: https://git.openjdk.org/jdk/pull/9515