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

[Leo Korinth]

Hi, comments inline!

On 27/05/2019 19:18, Kim Barrett wrote:
>> On May 24, 2019, at 7:25 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>
>> Hi,
>>
>> Here is the first patch in a proposed patch series of eight that
>> removes gcTaskManager and uses the WorkGang abstraction instead. This
>> is primary a refactoring to cleanup code for future
>> enhancements. After this change all GCs will use the WorkGang
>> abstraction.
> 
> Thanks for splitting this up into sections.  It’s definitely making reviewing easier.
> 
>> […]
>> # This Patch
>>
>> This first patch does two things:
>> 1) Adds work gang functionality
>>    + adds WorkGang named _workers to parallelScavengeHeap
>>    + sets active workers in psParallelCompact.cpp and
>>      psScavenge.cpp. The corresponding functionality in gcTaskManager
>>      is done by gc_task_manager()->set_active_gang()
>>
>> 2) Creates a PCRefProcTask that replaces RefProcTaskProxy and
>>    StealMarkingTask. The StealMarkingTask is removed in the next
>>    patch as it is still used by other parts.
>>
>> Enhancement:
>>   https://bugs.openjdk.java.net/browse/JDK-8224659
>>
>> Webrev:
>> http://cr.openjdk.java.net/~lkorinth/workgang/0/_8224659-Parallel-GC-Use-WorkGang-1-PCRefProcTask/
>>   http://cr.openjdk.java.net/~lkorinth/workgang/0/all/
>>
>> Testing (on the whole patch series):
>>   mach5 remote-build-and-test --build-profiles linux-x64,linux-x64-debug,macosx-x64,solaris-sparcv9,windows-x64 --test open/test/hotspot/jtreg/:hotspot_gc -a -XX:+UseParallelGC
>>   gc test suite
>>
>> Thanks,
>> Leo
> 
> ------------------------------------------------------------------------------
> 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?

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psParallelCompact.cpp
> 2108   do {
> 2109     while (ParCompactionManager::steal_objarray(worker_id,  task)) {
> 2110       cm->follow_array((objArrayOop)task.obj(), task.index());
> 2111       cm->follow_marking_stacks();
> 2112     }
> 2113     while (ParCompactionManager::steal(worker_id, obj)) {
> 2114       cm->follow_contents(obj);
> 2115       cm->follow_marking_stacks();
> 2116     }()this
> 2117   } while (!terminator.offer_termination());
> 
> This might offer termination earlier than it should, e.g. even if some
> objarrays have become available while other objects were being
> processed.  Consider instead something like
> 
>    while (true) {
>      if (ParCompactionManager::steal_objarray(worker_id,  task)) {
>        cm->follow_array((objArrayOop)task.obj(), task.index());
>        cm->follow_marking_stacks();
>      } else if (ParCompactionManager::steal(worker_id, obj)) {
>        cm->follow_contents(obj);
>        cm->follow_marking_stacks();
>      } else if (terminator.offer_termination()) {
>        break;
>      }
>    }
> 
> or maybe make the second clause
> 
>      } else if (ParCompactionManager::steal(worker_id, obj)) {
>        do {
>          cm->follow_contents(obj);
>          cm->follow_marking_stacks();
>        } while (ParCompactionManager::steal(worker_id, obj)) {
>      }
> 
> to only check for arrays again when run out of normal objects (as done
> in the original proposed change).

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

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

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

> 
> ------------------------------------------------------------------------------
> 

Thanks,
Leo