RFR: 8256814: WeakProcessorPhases may be redundant

[Kim Barrett]

> On Jan 12, 2021, at 5:12 AM, Stefan Karlsson <stefank at openjdk.java.net> wrote:
> 
> On Tue, 22 Dec 2020 04:59:28 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:
> 
>> Please review this change which eliminates the WeakProcessorPhase class.
>> 
>> The OopStorageSet class is changed to provide scoped enums for the different
>> categories: StrongId, WeakId, and Id (for the union of strong and weak).
>> An accessor is provided for obtaining the storage corresponding to a
>> category value.
>> 
>> Various other enumerator ranges, array sizes and indices, and iterations are
>> derived directly from the corresponding OopStorageSet category's enum range.
>> 
>> Iteration over a category of enumerators can be done via EnumIterator.  The
>> iteration over storage objects makes use of that enum iteration, rather than
>> having a bespoke implementation.  Some use-cases need iteration of the
>> enumerators, with storage lookup from the enumerator; other use-cases just
>> need the storage objects.
>> 
>> Testing:
>> mach5 tier1-6
>> Local (linux-x64) hotspot:tier1 with -XX:+UseShenandoahGC
> 
> I think this looks good. I have a few comments that I would like to get addressed, but they are not blockers if you want to proceed with what you have.

Thanks.

> src/hotspot/share/gc/shared/oopStorageSetParState.hpp line 35:
> 
>> 33: 
>> 34: // Base class for OopStorageSet{Strong,Weak}ParState.
>> 35: template<typename T, bool concurrent, bool is_const>
> 
> While reviewing this, it was not immediately obvious what T represent. EnumRange uses the name StorageId, maybe use the same here?

T -> StorageId -- done.

> src/hotspot/share/gc/shared/oopStorageSetParState.hpp line 52:
> 
>> 50: 
>> 51:   NONCOPYABLE(OopStorageSetParState);
>> 52: };
> 
> We tend to put the member variables at the top of classes. I don't think ParState needs to be public, and this could be changed to:
> […]

Having a public function whose type signature involves identifiers that
can't be used by clients, particularly for the return type, is problematic.

Personally, I intensely dislike the typical HotSpot ordering, but go along
anyway if there's not a direct reason not to, as there is here. (The HotSpot
ordering also happens to be contrary to every style guide I've ever seen
discuss the subject, and those style guides give good reasons that are the
basis of my dislike.)

Options are (1) drop the type alias and write out the type, (2) have
multiple public sections, (3) put the public stuff first. I prefer (3).
Though I went with (2) in weakProcessorTimes.hpp, to reduce the code churn
for this PR.

> src/hotspot/share/gc/shared/oopStorageSetParState.hpp line 58:
> 
>> 56: class OopStorageSetStrongParState
>> 57:   : public OopStorageSetParState<OopStorageSet::StrongId, concurrent, is_const>
>> 58: {
> 
> We usually keep the `{` on the same line.

We also usually put the base class on the same line as the class, but that
made the lines longer than I like, hence the line break there.  Having the
open brace at the end of the base class line then makes the distinction
between the base class part and the members more subtle than I liked; I think
the brace placement I used helps with that.  (All this is a long-winded way
of saying that the formatting here is intentional, attempting to make the code
easier to parse by eye.)

> src/hotspot/share/gc/shared/oopStorageSetParState.hpp line 68:
> 
>> 66: class OopStorageSetWeakParState
>> 67:   : public OopStorageSetParState<OopStorageSet::WeakId, concurrent, is_const>
>> 68: {
> 
> Same comment as above.
> 
> src/hotspot/share/gc/shared/oopStorageSetParState.inline.hpp line 36:
> 
>> 34: 
>> 35: template<bool concurrent, bool is_const>
>> 36: template<typename Closure>
> 
> Other places in the file uses `template <` so the usage of `template<` makes the code inconsistent.

Yeah, sorry, I try to be consistent with nearby code but failed here; fixed.

FWIW, HotSpot uses both, and there doesn't seem to be a consensus in the
wider C++ community. The C++ Standard uses both! My default has always been
no-space, and having a space there bugs me a little, but not enough to argue
over.

> src/hotspot/share/gc/shared/weakProcessorTimes.hpp line 37:
> 
>> 35: class WeakProcessorTimes {
>> 36: public:
>> 37:   using StorageId = OopStorageSet::WeakId;
> 
> Could be private.

Here too I think public functions whose type signatures involve identifiers
that can't be used by clients is problematic.  But I renamed it from
StorageId to WeakId; looking at it again, the more generic name seems
counterproductive here.

> test/hotspot/gtest/gc/shared/test_oopStorageSet.cpp line 48:
> 
>> 46: 
>> 47:   template <uint count>
>> 48:   static void check_iterator(OopStorageSet::Iterator it,
> 
> All the functions you changed are named `_iterator` and tested OopStorageSet::Iterator. Now the name is the same, but instead they test the Range facility. I think these functions should be renamed. Alternatively, we keep the tests for the OopStorageSet::Iterator and create a new set for the Range?

What's being tested is iteration, so "iterator" => "iteration” throughout seems better.

> Marked as reviewed by stefank (Reviewer).

Thanks for reviewing.

> PR: https://git.openjdk.java.net/jdk/pull/1862