RFR: 8224660: Parallel GC: Use WorkGang (2: MarksFromRootsTask)

[Kim Barrett]

> On May 29, 2019, at 6:20 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>> src/hotspot/share/gc/parallel/psRootType.inline.hpp
>> 32 namespace Parallel { namespace RootType {
>> We mostly don't use namespaces in HotSpot code (the style guide
>> forbids them, probably at least in part for obsolete reasons).  I
>> don't see any reason for violating that dictum here, rather than just
>> using AllStatic classes.
> 
> One good reason is that it is hard to put stuff in a huge "Parallel" AllStatic class. I am attempting to change the style guide regarding suggestions of not using namespaces. I will put it in an AllStatic and name it ParallelRootType if you feel it is /better/, do you think that?

But why would we need a huge "Parallel" AllStatic class?  We can just
continue to use prefix naming conventions, like generations of C ;-)
programmers before us.

If we were starting HotSpot today, I'd be in favor of using
namespaces.  I just don't see the benefit of introducing them now,
except in a few specific cases (like os, or if actively wanting ADL).

ParallelRootType looks good to me.


>> src/hotspot/share/gc/parallel/psRootType.inline.hpp
>> 53   EnumClaimer(T to_exclusive) : _counter(0), _to_exclusive(to_exclusive) {
> 
> Thanks, I will take a look at PrimitiveConversions::cast.

I just noticed that the comment describing PrimitiveConversions::cast
forgot to include enum in the list.  (enums are not integral types.)
That's a bug.

>> src/hotspot/share/gc/parallel/psRootType.inline.hpp
>> Is EnumClaimer enough better than the existing mechanism of an
>> if-sequence on SubTasksDone::try_claim_task to be worth having?
>> One thing I dislike is the need for a sentinal enumerator.  Of course,
>> I don't much like the SubTasksDone::all_tasks_completed protocol
>> either; I have some notes about it that should be turned into an RFE.
> 
> I prefer it because it tries to be typesafe and does not have all_tasks_completed which in this case is not needed. I would prefer having a separate barrier for when it is needed.

all_tasks_completed exists to support reuse of a SubTasksDone object.
It would be better to just disallow such reuse; it's so not worth the
added usage complexity.  My notes say the only place with a reused
SubTasksDone is GenCollectedHeap, which I think therefore means CMS
(since Serial doesn't have parallelism, but is code-sharing with CMS
here).  I haven't looked at how hard it would be to kill that re-use.

>> Assuming we're going to use it (and replace existing SubTasksDone
>> usages with it?), the class really should be in gc/shared, probably in
>> workgroup.hpp (having the class declaration in a .inline.hpp prevents
>> embedding an instance in some other class that is declared in a .hpp),
>> with the implementation in (new) workgroup.inline.hpp >
>> And the Parallel::RootType::Value enum should be in psRootType.hpp.
> 
> I did put it in workgroup to begin with and moved it out (after suggestions from colleagues) as I were the only user of it. Do you think it would be okay to leave it in the .inline.hpp file (as is) and if we find more users of the class in the future maybe move it to some shared class (and then separate it into hpp and cpp file)? I feel splitting it is overkill and hurts readability when the interface is never needed without the implementation.

What I really want to avoid is having two facilities for solving the
same problem.  That's why I suggested a different location.  If the
plan is to replace SubTasksDone with EnumClaimer, it should be in
shared code.  If that's not the plan, then I don't want EnumClaimer at
all.