RFR: 8224659: Parallel GC: Use WorkGang (1: PCRefProcTask)

[Kim Barrett]

> On May 29, 2019, at 8:12 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
> On 27/05/2019 19:18, Kim Barrett wrote:
>> src/hotspot/share/gc/parallel/psParallelCompact.cpp
>> 2100 void steal_marking_task(ParallelTaskTerminator& terminator, uint worker_id) {
>> Should be static (file-scoped).  That might also obviate the initial
>> assertion that it is called during a GC.
> 
> Yes, I will make it static as well as all other *_task methods I have created in later patches. I got off-list feedback on this, and I do not know why I missed it. Sorry.
> 
> The assertion is old, sometimes people get angry when asserts are removed. I have no strong opinion on my own here. I have kept all old asserts (except multiple redundant asserts when I have merged functions). Do you want it to go away?

No particular preference.  It just seems a bit pointless, given the
sole caller is right there just a few lines later.

> I have created https://bugs.openjdk.java.net/browse/JDK-8224976

Thanks.

>> src/hotspot/share/gc/parallel/psParallelCompact.cpp
>> 2103   ParCompactionManager* cm =
>> 2104     ParCompactionManager::gc_thread_compaction_manager(worker_id);
>> Rather than making a new manager here, why not use the one from the
>> caller, having it passed to this helper function as an additional
>> argument?
> 
> I am not making a new manager, just fetching an old one. I could pass it as a parameter (I kind of agree that the code would look better), but the code would look less like before. I would like to keep it, but I have no problem changing it if you feel a parameter makes a difference.

Oops, I missed that is's a pointer.  In that case I have no strong preference.

>> src/hotspot/share/gc/parallel/psParallelCompact.cpp
>> 1793     ParallelScavengeHeap::heap()->workers().update_active_workers(WorkerPolicy::calc_active_workers(
>> 1794       ParallelScavengeHeap::heap()->workers().total_workers(),
>> 1795       ParallelScavengeHeap::heap()->workers().active_workers(),
>> 1796       Threads::number_of_non_daemon_threads()));
>> This appears again in src/hotspot/share/gc/parallel/psScavenge.cpp (in
>> PSScavenge::invoke_no_policy()):
>> 342     uint active_workers = ParallelScavengeHeap::heap()->workers().update_active_workers(WorkerPolicy::calc_active_workers(
>> 343       ParallelScavengeHeap::heap()->workers().total_workers(),
>> 344       ParallelScavengeHeap::heap()->workers().active_workers(),
>> 345       Threads::number_of_non_daemon_threads()));
>> Perhaps there should be a helper for this?  Or do things change later
>> in the sequence of changesets?
> 
> I did a helper in WorkGang before, but removed it because I got some feedback that the work gang should not be involved with the worker policy, which I agree with. I think you are right in that an abstraction would be good though, maybe a method in the worker policy code that takes a WorkGang as argument?
> 
> However I think this should not be fixed here, and instead be added in a separate patch as it is not specific to parallel. The same behaviour is used in G1 (a bit different in the full collector) and CMS as well.

OK.