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

[Leo Korinth]

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

Okay, Will fix.

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

Sorry, I was answering your older question a bit too fast as I thought 
you were talking about *Sequential*SubTasksDone. I would _not_ like to 
use SubTasksDone as it lacks the same features SequentialSubTasksDone 
lacks plus it _also_ allocates memory.

SequentialSubTasksDone has all_tasks_completed (in addition to object 
reuse as you noted) to signal that cleanup can be made on resources by 
the last thread (outside the class). It returns a boolean that is 
checked in one place in CMS. Reuse of SequentialSubTasksDone just seems 
like bad design as it does not even allocate memory (and thus is free to 
create). However it is re-used and located in Space for (to me) no 
apparent reason, especially when it is a StackObj.


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

I think I do understand what you want to avoid (that was why I did 
initially place it in shared hoping to remove SequentialSubTasksDone 
eventually). However I also understand why Per and StefanK do not want 
to place every new class "that could be used in other places" in shared 
code. I also feel it is no good to "fix" SequentialSubTasksDone, fix its 
usage in other GCs, and do all that work in this -- already -- quite big 
patch series. My guess is that _that_ would not be acceptable to quite a 
few reviewers.

As my proposed solution is not accepted, I guess the way forward is to 
kill my EnumClaimer, use SequentialSubTasksDone, manually cast to enum 
values and after that call all_tasks_completed(). all_tasks_completed() 
is of course not needed in my case (it will just eat cpu cycles), but it 
is documented that it must be called, and I do not want to break the 
contract. Are you okay with such a solution? I do not like it myself, 
but it might be a not-so-good solution that everyone is able to accept. 
Do you have a better proposal?

Sometimes it feels that we all want the perfect solution and by avoiding 
a non-perfect solution that is, IMO, quite a bit better than before, we 
end up reverting to using the the old inferior one and in the process 
loosing an incremental improvement. I will now probably add another use 
of what is in my opinion a bad class. And later in this patch series I 
will add another usage to that class. SequentialSubTasksDone will thus 
have two more usages and be harder to remove after this. People will 
also more likely find the class and see that this is a class that is 
used in many places in the code base and thus use it.

Thanks,
Leo