RFR: 8256814: WeakProcessorPhases may be redundant

[Stefan Karlsson]

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.

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?

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:
template<typename T, bool concurrent, bool is_const>
class OopStorageSetParState {
  using ParState = OopStorage::ParState<concurrent, is_const>;

  ValueObjArray<ParState, EnumRange<T>().size()> _par_states;

public:
  ParState* par_state(T id) const {
    return _par_states.at(checked_cast<int>(EnumRange<T>().index(id)));
  }

protected:
  OopStorageSetParState() : _par_states(OopStorageSet::Range<T>().begin()) {}
  ~OopStorageSetParState() = default;

private:
  NONCOPYABLE(OopStorageSetParState);
};

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.

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.

src/hotspot/share/gc/shared/weakProcessorTimes.hpp line 37:

> 35: class WeakProcessorTimes {
> 36: public:
> 37:   using StorageId = OopStorageSet::WeakId;

Could be private.

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?

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

Marked as reviewed by stefank (Reviewer).

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