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

[Leo Korinth]

Hi!

On 20/07/2019 04:12, Kim Barrett wrote:
>> 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 :)

Will correct my "fix".

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psScavenge.cpp
> 337 };
> 338 //
> 339 // OldToYoungRootsTask
> 
> Add a blank line between the class and the block comment.

Will fix.

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

Will fix.

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

The comment is taken (without changes) from psTasks.hpp and describes 
the first part (OldToYoungRootsTask). The new task ScavengeRootsTask now 
does three things from the old code:
- OldToYoungRootsTask (where the comment is from)
- ScavengeRootsTask
- StealTask (depending on thread count)

I guess the problem could be bad naming from my side, maybe the name of 
ScavengeRootsTask ought to reflect this "merge". Maybe I should rename 
the task ScavengeRootsTask and maybe I should extract the method 
old_to_young_roots_task() and place the comment there? Maybe the comment 
is not needed at all?

How would you prefer to have it?

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

I believe the comment is still valid, the logic is meant to be the same. 
Do you think the comment is better if I s/Str?ealTask/steal_task() or 
have I missed something? Because no change in the behaviour was done on 
purpose.

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

Sorry Kim, I do not understand what you suggest here. Are you referring 
to the order of the members of enum ParallelRootType, and thus the order 
in which they are dispatched (in parallel)?

Thanks,
Leo

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