RFR: 8224663: Parallel GC: Use WorkGang (5: ScavengeRootsTask)

[Kim Barrett]

> On May 27, 2019, at 12:30 PM, Leo Korinth <leo.korinth at oracle.com> wrote:
> 
> Hi,
> 
> Here is the fifth patch in a proposed patch series of eight that
> removes gcTaskManager and uses the WorkGang abstraction instead.
> 
> ScavengeRootsTask, ThreadRootsTask and OldToYoungRootsTask is replaced
> with ScavengeRootsTask. Code is basically the same, the major
> difference is that roots are visited using EnumClaimer and
> Threads::possibly_parallel_threads_do. Here we can reuse the RootType
> and EnumClaimer from patch number two.
> 
> The reason "case threads:" was removed is that the code is dead. That
> part is confusing as the code is done (in parallel from the calling
> function).
> 
> Enhancement:
>  https://bugs.openjdk.java.net/browse/JDK-8224663
> 
> Webrev:
> http://cr.openjdk.java.net/~lkorinth/workgang/0/_8224663-Parallel-GC-Use-WorkGang-5-ScavengeRootsTask/
>  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

8224663-Parallel-GC-Use-WorkGang-5-ScavengeRootsTask
8224663-Parallel-GC-Use-WorkGang-5-ScavengeRootsTask-fixup-1
8224663-Parallel-GC-Use-WorkGang-5-ScavengeRootsTask-fixup-2

Looks good, other than a couple formatting nits and some stale comments.

------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/psScavenge.cpp
55 #if INCLUDE_JVMCI
56 #include "jvmci/jvmci.hpp"
57 #endif

Conditional includes go at the end, per the style guide. There are
probably counter-examples :)

------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/psScavenge.cpp
337 };
338 //
339 // OldToYoungRootsTask

Add a blank line between the class and the block comment.

------------------------------------------------------------------------------ 
393   ScavengeRootsTask(
394     PSOldGen* old_gen,
395     HeapWord* gen_top,
396     uint active_workers,
397     bool is_empty) :

It's more usual to line these up with the "(" with the first parameter
on the same line as the function name.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/parallel/psScavenge.cpp  
339 // OldToYoungRootsTask
340 //
341 // This task is used to scan old to young roots in parallel

This comment seems like it needs some update, and maybe is misplaced?
This seems like it's really a description of scavenge_contents_parallel,
in conjunction with the old task creation model.

------------------------------------------------------------------------------ 
src/hotspot/share/gc/parallel/psScavenge.cpp  
446     // If active_workers can exceed 1, add a StrealTask.
447     // PSPromotionManager::drain_stacks_depth() does not fully drain its
448     // stacks and expects a StealTask to complete the draining if
449     // ParallelGCThreads is > 1.

Stale comment now?

------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/psScavenge.cpp  
438     for (Parallel::RootType::Value root_type; _enum_claimer.try_claim(root_type); /* empty */) {
439       scavenge_roots_task(root_type, worker_id);
440     }

For the future, maybe serial processing phases should be moved
earlier.  OK to leave it for now to maintain correlation with the old
code.

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