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

[Kim Barrett]

> On Aug 5, 2019, at 11:41 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
> 
> 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?

OldToYoungRootsTask basically consisted of the call to
PSCardTable::scavenge_contents_parallel, and this comment really seems
to be about how that function works.  Maybe it should be moved there,
and tidied up for that new location.

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

Oh, I see.  The comment's reference to PSPM::drain_stacks_depth()
really should be to the call to drain_stacks(false) earlier in the function
containing the comment.  (Which were in different functions before
your changes, so yay for better co-location of code and comments.)
[pre-existing]

There's no such thing as a StealTask anymore.  It should be referring
to steal_task().

In workgang nomenclature, scavenge_roots_task and steal_task are
perhaps misnamed.  They aren't tasks, they are helper work functions.
Maybe they should be called scavenge_roots_work and steal_work?

I don't remember if there were similar possible naming issues
elsewhere in this cluster of changes.  And you should verify the
naming convention with someone else before making any changes in
response to this comment.

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

I was wondering if the serial subtasks in scavenge_roots_task might
not be better scheduled before card_table->scavenge_contents_parallel().
But now that I better understand how the latter works, I no longer
think so.

I think we already talked about the order of the ParallelRootType enumerators
and how that might be important for scheduling, and agreed that could be
looked at later.

And finally, a couple more minor things that I missed earlier, both pre-existing.

------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/psScavenge.cpp
416       assert(!_old_gen->object_space()->is_empty(),
417         "Should not be called is there is no work");
418       assert(_old_gen != NULL, "Sanity");

[pre-existing]
Checking _old_gen != NULL after already using it in the previous
assert is kind of pointless.  These asserts should be reordered.

------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/psScavenge.cpp
381 // to the start of stride 0 in slice 1.

[pre-existing]
s/stride/stripe/

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