8256814: WeakProcessorPhases may be redundant by kimbarrett · Pull Request #1862 · openjdk/jdk

kimbarrett · 2020-12-22T04:59:28Z

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8256814: WeakProcessorPhases may be redundant

Reviewers

Stefan Karlsson (@stefank - Reviewer) ⚠️ Review applies to 8d6c590
Thomas Schatzl (@tschatzl - Reviewer) ⚠️ Review applies to 3a4d5b7
Roman Kennke (@rkennke - Reviewer) ⚠️ Review applies to ebe50e3

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1862/head:pull/1862
$ git checkout pull/1862

…eSet.

bridgekeeper · 2020-12-22T05:00:17Z

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2020-12-22T05:00:48Z

@kimbarrett Unknown command labels - for a list of valid commands use /help.

openjdk · 2020-12-22T05:01:06Z

@kimbarrett The following labels will be automatically applied to this pull request:

hotspot
shenandoah

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

kimbarrett · 2020-12-22T05:02:05Z

/summary Remove WeakProcessorPhase, adding scoped enum categories to OopStorageSet.

openjdk · 2020-12-22T05:02:19Z

@kimbarrett Setting summary to Remove WeakProcessorPhase, adding scoped enum categories to OopStorageSet.

kimbarrett · 2020-12-22T05:03:20Z

/label remove hotspot
/label add hotspot-gc
/label add hotspot-runtime

openjdk · 2020-12-22T05:03:36Z

@kimbarrett
The hotspot label was successfully removed.

openjdk · 2020-12-22T05:04:07Z

@kimbarrett
The hotspot-gc label was successfully added.

kimbarrett · 2020-12-22T05:04:27Z

/label add jfr

openjdk · 2020-12-22T05:04:39Z

@kimbarrett
The hotspot-runtime label was successfully added.

openjdk · 2020-12-22T05:05:07Z

@kimbarrett The label jfr is not a valid label. These labels are valid:

serviceability
hotspot
sound
hotspot-compiler
kulla
i18n
shenandoah
jdk
javadoc
2d
security
swing
hotspot-runtime
jmx
build
nio
beans
core-libs
compiler
net
hotspot-gc
hotspot-jfr
awt

kimbarrett · 2020-12-22T05:05:36Z

/label add hotspot-jfr

openjdk · 2020-12-22T05:06:04Z

@kimbarrett
The hotspot-jfr label was successfully added.

mlbridge · 2020-12-22T05:10:07Z

Webrevs

stefank

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.

stefank · 2021-01-12T09:02:02Z

src/hotspot/share/gc/shared/oopStorageSetParState.hpp

-  ParStateArray _par_states;
-
+// Base class for OopStorageSet{Strong,Weak}ParState.
+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?

stefank · 2021-01-12T09:31:02Z

src/hotspot/share/gc/shared/oopStorageSetParState.hpp

-  typedef ValueObjArray<ParStateType, OopStorageSet::strong_count> ParStateArray;
-
-  ParStateArray _par_states;
-
+// Base class for OopStorageSet{Strong,Weak}ParState.
+template<typename T, bool concurrent, bool is_const>
+class OopStorageSetParState {
 public:
-  OopStorageSetStrongParState();
+  using ParState = OopStorage::ParState<concurrent, is_const>;

-  template <typename Closure>
-  void oops_do(Closure* cl);
+  ParState* par_state(T id) const {
+    return _par_states.at(checked_cast<int>(EnumRange<T>().index(id)));
+  }

-  ParStateType* par_state(int i) const { return _par_states.at(i); }
-  int par_state_count() const { return _par_states.count(); }
-};
+protected:
+  OopStorageSetParState() : _par_states(OopStorageSet::Range<T>().begin()) {}
+  ~OopStorageSetParState() = default;

-template <bool concurrent, bool is_const>
-class OopStorageSetWeakParState {
-  typedef OopStorage::ParState<concurrent, is_const> ParStateType;
-  typedef ValueObjArray<ParStateType, OopStorageSet::weak_count> ParStateArray;
+private:
+  ValueObjArray<ParState, EnumRange<T>().size()> _par_states;

-  ParStateArray _par_states;
+  NONCOPYABLE(OopStorageSetParState);
+};


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); };

stefank · 2021-01-12T09:33:12Z

src/hotspot/share/gc/shared/oopStorageSetParState.hpp

+  : public OopStorageSetParState<OopStorageSet::StrongId, concurrent, is_const>
+{


We usually keep the { on the same line.

stefank · 2021-01-12T09:33:21Z

src/hotspot/share/gc/shared/oopStorageSetParState.hpp

+template<bool concurrent, bool is_const>
+class OopStorageSetWeakParState
+  : public OopStorageSetParState<OopStorageSet::WeakId, concurrent, is_const>
+{


Same comment as above.

stefank · 2021-01-12T09:34:58Z

src/hotspot/share/gc/shared/oopStorageSetParState.inline.hpp

+template<bool concurrent, bool is_const>
+template<typename Closure>


Other places in the file uses template < so the usage of template< makes the code inconsistent.

stefank · 2021-01-12T09:46:03Z

src/hotspot/share/gc/shared/weakProcessorTimes.hpp

+class WeakProcessorTimes {
+public:
+  using StorageId = OopStorageSet::WeakId;


Could be private.

stefank · 2021-01-12T10:07:45Z

test/hotspot/gtest/gc/shared/test_oopStorageSet.cpp

-  template <uint count>
-  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?

openjdk · 2021-01-12T10:10:35Z

@kimbarrett this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout wpp4
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2021-01-16T15:39:38Z

@kimbarrett This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256814: WeakProcessorPhases may be redundant

Remove WeakProcessorPhase, adding scoped enum categories to OopStorageSet.

Reviewed-by: stefank, tschatzl, rkennke

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

mlbridge · 2021-01-16T15:43:39Z

Mailing list message from Kim Barrett on shenandoah-dev:

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.

tschatzl · 2021-01-18T10:27:28Z

src/hotspot/share/gc/shared/oopStorageSet.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2019, 2020, Oracle and/or its affiliates. All rights reserved.


It's 2021 by now, I suggest to update these before pushing.

mlbridge · 2021-01-18T10:43:45Z

Mailing list message from Kim Barrett on hotspot-jfr-dev:

On Jan 16, 2021, at 10:42 AM, Kim Barrett <kim.barrett at oracle.com> wrote:

On Jan 12, 2021, at 5:12 AM, Stefan Karlsson <stefank at openjdk.java.net> wrote:
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.

After thinking about this some more, I?m going to see what it looks like to just
eliminate the type alias entirely.

mlbridge · 2021-01-18T15:15:44Z

Mailing list message from Kim Barrett on hotspot-jfr-dev:

On Jan 18, 2021, at 5:41 AM, Kim Barrett <kim.barrett at oracle.com> wrote:

On Jan 16, 2021, at 10:42 AM, Kim Barrett <kim.barrett at oracle.com> wrote:

On Jan 12, 2021, at 5:12 AM, Stefan Karlsson <stefank at openjdk.java.net> wrote:
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.

After thinking about this some more, I?m going to see what it looks like to just
eliminate the type alias entirely.

Done. The type alias didn?t really add much, just shortening some a few uses and
signatures, not really enough to justify adding it.

I also updated the copyrights for 2021.

rkennke

Changes look good to me! I also ran some tests with Shenandoah and they look good too! Thanks!

kimbarrett · 2021-01-22T13:12:13Z

Thanks @stefank , @tschatzl , @rkennke for reviews.

/integrate

openjdk · 2021-01-22T13:13:10Z

@kimbarrett Pushed as commit 7ed8ba1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Remove WeakProcessorPhase, adding scoped enum categories to OopStorag…

8d6c590

…eSet.

openjdk bot added hotspot hotspot-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org labels Dec 22, 2020

openjdk bot removed the hotspot hotspot-dev@openjdk.org label Dec 22, 2020

openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Dec 22, 2020

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 22, 2020

openjdk bot added the hotspot-jfr hotspot-jfr-dev@openjdk.org label Dec 22, 2020

kimbarrett marked this pull request as ready for review December 22, 2020 05:06

openjdk bot added the rfr Pull request is ready for review label Dec 22, 2020

stefank approved these changes Jan 12, 2021

View reviewed changes

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jan 12, 2021

kimbarrett added 2 commits January 16, 2021 09:50

stefank review

ce4711c

Merge branch 'master' into wpp4

3a4d5b7

openjdk bot added the ready Pull request is ready to be integrated label Jan 16, 2021

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Jan 16, 2021

tschatzl approved these changes Jan 18, 2021

View reviewed changes

kimbarrett added 2 commits January 18, 2021 09:37

remove type aliases for OopStorageSet::WeakId

6fe8cd4

update copyrights

ebe50e3

rkennke approved these changes Jan 18, 2021

View reviewed changes

Merge branch 'master' into wpp4

0980bd2

openjdk bot closed this Jan 22, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 22, 2021

kimbarrett deleted the wpp4 branch January 22, 2021 13:14

		: public OopStorageSetParState<OopStorageSet::StrongId, concurrent, is_const>
		{

		template<bool concurrent, bool is_const>
		template<typename Closure>

		template <uint count>
		static void check_iterator(OopStorageSet::Iterator it,

Conversation

kimbarrett commented Dec 22, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Dec 22, 2020

Uh oh!

openjdk bot commented Dec 22, 2020

Uh oh!

openjdk bot commented Dec 22, 2020

Uh oh!

kimbarrett commented Dec 22, 2020

Uh oh!

openjdk bot commented Dec 22, 2020

Uh oh!

kimbarrett commented Dec 22, 2020

Uh oh!

openjdk bot commented Dec 22, 2020

Uh oh!

openjdk bot commented Dec 22, 2020

Uh oh!

kimbarrett commented Dec 22, 2020

Uh oh!

openjdk bot commented Dec 22, 2020

Uh oh!

openjdk bot commented Dec 22, 2020

Uh oh!

kimbarrett commented Dec 22, 2020

Uh oh!

openjdk bot commented Dec 22, 2020

Uh oh!

mlbridge bot commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

stefank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 12, 2021

Uh oh!

openjdk bot commented Jan 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Jan 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Jan 18, 2021

Uh oh!

mlbridge bot commented Jan 18, 2021

Uh oh!

rkennke left a comment

Choose a reason for hiding this comment

Uh oh!

kimbarrett commented Jan 22, 2021

Uh oh!

openjdk bot commented Jan 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

kimbarrett commented Dec 22, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 22, 2020 •

edited

Loading

openjdk bot commented Jan 16, 2021 •

edited

Loading