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

[Leo Korinth]

Hi, comments inlined below...

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

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psRootType.inline.hpp
> +      enum_reference = reinterpret_cast<T&>(claimed);
> 
> We have PrimitiveConversions::cast for this.  (Until C++11+ that will
> require registering the enum; see metaprogramming/isRegisteredEnum.hpp.)
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psRootType.inline.hpp
> 53   EnumClaimer(T to_exclusive) : _counter(0), _to_exclusive(to_exclusive) {
> 
> Using PrimitiveConversions::cast to explicitly convert to_exclusive
> for initialization of _to_exclusive is future-proof against making the
> enum a C++11 enum class.
> 
> Again here, requires registering the enum until C++11+.
> 
> ------------------------------------------------------------------------------
> 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.

> Should be "explicit" to disable implicit conversion.

I agree (and I will fix), but it does force you to use a sentinel as you 
can not do enum_value_last_inclusive + 1 (see your next point).

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

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

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psRootType.inline.hpp
> 58     assert(_counter >= _to_exclusive, "not all enumerations were claimed");
> 
> s/enumerations/enumerators/

Will fix!

> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psParallelCompact.cpp
> 2203   MarkFromRootsTask(uint active_workers)
> 2204     : AbstractGangTask("MarkFromRootsTask"),
> 
> Typical HotSpot style puts the colon at the end of the parameter list,
> and indents the initializer list either 2 spaces (or 4 spaces in some
> recent code, mostly in ZGC).

Will fix.
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/parallel/psParallelCompact.cpp
> 2211     for (Parallel::RootType::Value root_type; _enum_claimer.try_claim(root_type); /* empty */) {
> 2212         mark_from_roots_task(root_type, worker_id);
> 
> and
> 
> 2218     if (_active_workers > 1) {
> 2219        steal_marking_task(*_terminator.terminator(), worker_id);
> 
> Unusual indentation.
will fix.
> 
> ------------------------------------------------------------------------------
> 
Thanks,
Leo