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

[Kim Barrett]

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

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

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

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

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