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

Tue Aug 6 08:17:15 UTC 2019

On 05/08/2019 23:51, Kim Barrett wrote:
>> 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.

Okay. Will try to do that.

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

You are probably right.

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

I have named all these "*_work" functions "*_task" consistently through 
all patches to reflect the old usage of the code :-(, I will rename them 
if Thomas agree, is that okay with you Thomas?

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

Okay, will not change then.

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

Yes, the order should obviously be changed. Nice catch!

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psScavenge.cpp
> 381 // to the start of stride 0 in slice 1.
> 
> [pre-existing]
> s/stride/stripe/

Will fix.

Thanks,
Leo

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