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

[Kim Barrett]

> On May 24, 2019, at 9:54 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
> 
> Hi,
> 
> Here is the second patch in a proposed patch series of eight that
> removes gcTaskManager and uses the WorkGang abstraction instead. I will try to put these mails in the same mail thread.
> 
> This patch moves the  MarkFromRootsTask, ThreadRootsMarkingTask and
> the StealMarkingTask (the last one we already moved in the last patch).
> They are all called from the new MarkFromRootsTask.
> 
> The RootType enum as well as EnumClaimer, a class that can claim
> enumeration values atomically is put in an inline header file. It will
> be used in later patches outside psParallelCompact if you wonder about
> the header file placement.
> 
> Enhancement:
>  https://bugs.openjdk.java.net/browse/JDK-8224660
> 
> Webrev:
> http://cr.openjdk.java.net/~lkorinth/workgang/0/_8224660-Parallel-GC-Use-WorkGang-2-MarksFromRootsTask/
>  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

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

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

Should be "explicit" to disable implicit conversion.

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

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.

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

s/enumerations/enumerators/

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

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

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