RFR: Parallelize safepoint cleanup

[Roman Kennke]

Am 30.05.2017 um 03:59 schrieb David Holmes:
> Hi Roman,
>
> On 29/05/2017 6:23 PM, Roman Kennke wrote:
>> Hi David,
>>
>> the patch applied cleanly, i.e. was not affected by any hotspot changes.
>> Most of your suggestions applied to the GC part that I excluded from the
>> scope of this change, except for the following:
>>
>>>> I introduced a dedicated worker thread pool in SafepointSynchronize.
>>>> This is only initialized when -XX:+ParallelSafepointCleanup is
>>>> enabled,
>>>> and uses -XX:ParallelSafepointCleanupThreads=X threads, defaulting
>>>> to 8
>>>> threads (just a wild guess, open for discussion. With
>>>> -XX:-ParallelSafepointCleanup turned off (the default) it will use the
>>>> old serial safepoint cleanup processing (with optional GC hooks, see
>>>> below).
>>>
>>> I think the default needs to be ergonomically set, as for other
>>> "thread pools". Both flags should be experimental at this stage.
>>
>> I implemented this using the following:
>>
>>    if (ParallelSafepointCleanup &&
>> FLAG_IS_DEFAULT(ParallelSafepointCleanupThreads)) {
>>      // This will pick up ParallelGCThreads if set...
>>      FLAG_SET_DEFAULT(ParallelSafepointCleanupThreads,
>> Abstract_VM_Version::parallel_worker_threads());
>>    }
>>
>> As noted, it will pick up ParallelGCThreads, which might be a reasonable
>> number. Otherwise we'd need to re-work the logic in vm_version.cpp to
>> calculate the number of worker threads without considering
>> ParallelGCThreads.
>
> I think this is way too big a number of threads to consider for this
> situation. GC has a huge amount of work to do, but the safepoint
> cleanup should be much less work.
>
> I'm more inclined now to set a small hard-wired default initially -
> say 4 - and for future work look at generalising the existing fixed GC
> thread pool into a more general purpose, dynamic thread pool
> (something similar to java.util.concurrent.ThreadPoolExecutor.) I note
> others object to a dedicated thread pool for this, so generalising the
> existing one seems to be a reasonable future direction, whilst
> avoiding tying this to specific GCs.

ok, I changed the following:
- default to 4 safepoint cleanup threads, no ergo
- added a hook into the GC, which can now share its worker threads with
safepoint cleanup. works for CMS and G1, Parallel does not use WorkGang,
Serial doesn't use multithreads at all. GC can return NULL, which lets
the safepoint code create its own worker threads (when
ParallelSafepointCleanup is requested). When using GC workers, only
min(ParallelSafepointCleanupThread, ParallelGCThreads) are used.
- serial and parallel code paths are now the same. Serial is calling the
task from the VMThread. Pro: no rarely tested code paths, con: serial
uses claiming logic. I'm undecided: if you're favoring to keep separate
code paths, let me know.

Testing: hotspot_gc all passes with release and fastdebug. no
regressions with specjvm

http://cr.openjdk.java.net/~rkennke/8180932/webrev.03/
<http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.03/>

Roman