8253402: Convert vmSymbols::SID to enum class #276

iklam · 2020-09-21T07:07:25Z

Convert vmSymbols::SID to an enum class to provide better type safety.

The original enum type vmSymbols::SID cannot be forward-declared. I moved it out of the vmSymbols class and renamed, so now it can be forward-declared as enum class vmSymbolID : int;, without including the large vmSymbols.hpp file.
- This also breaks the mutual dependency between the vmSymbols and vmIntrinsics classes. Now the declaration of vmIntrinsics can be moved from vmSymbols.hpp to vmIntrinsics.hpp, where it naturally belongs.
Type-safe enumeration (contributed by Kim Barrett)

for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) {
 vmSymbolID index = *it; ....
}

I moved vmSymbols::_symbols[] to Symbol::_vm_symbols[], and made it accessible via Symbol::vm_symbol_at(). This way, header files (e.g. fieldInfo.hpp) that need to convert from vmSymbolID to Symbol* don't need to include the large vmSymbols.hpp file.
I changed the VM_SYMBOL_ENUM_NAME macro so that the users don't need to explicitly add the vmSymbolID:: scope.
I removed many unnecessary casts between int and vmSymbolID.
The remaining casts are done via vmSymbol::as_int() and vmSymbols::as_SID() with range checks.

If this is successful, I will do the same for vmIntrinsics::ID.

Progress

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

Issue

JDK-8253402: Convert vmSymbols::SID to enum class

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 9ddca08
Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 443cf52
Kim Barrett (@kimbarrett - Reviewer) ⚠️ Review applies to 662ad8b
Igor Veresov (@veresov - Reviewer) ⚠️ Review applies to 9ddca08

Download

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

bridgekeeper · 2020-09-21T07:09:27Z

👋 Welcome back iklam! 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-09-21T07:10:47Z

@iklam The following labels will be automatically applied to this pull request: hotspot serviceability.

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 (add|remove) "label" command.

iklam · 2020-09-21T07:15:00Z

/label remove serviceability

openjdk · 2020-09-21T07:15:26Z

@iklam
The serviceability label was successfully removed.

mlbridge · 2020-09-21T07:21:58Z

Webrevs

vnkozlov · 2020-09-23T16:38:39Z

src/hotspot/share/ci/ciObjectFactory.hpp

@@ -29,6 +29,8 @@
 #include "ci/ciObject.hpp"
 #include "utilities/growableArray.hpp"

+enum class vmSymbolID : int;


Do you need this declaration to avoid #include "classfile/vmSymbols.hpp" ?

Yes, that's the intention, because vmSymbols.hpp is big.

I am thinking whether I should move such forward declarations of enums into globalDefinitions.hpp, so header files that use vmSymbolID don't need to know that the base type is int.

No, don't move them to globalDefinitions.hpp, only have the forward declarations where they're used. So this is like a class forward declaration? neat.

How about having a new header file utilities/vmEnums.hpp that looks like this:

// Include this header file if you just need the following enum types and // you don't use their members directly. This way you don't need to include the // complex header files that have the full definitions of these enums. enum JVMFlagsEnum : int; enum class vmSymbolID : int;

My plan is to have at least 2 more such enums (one for vmIntrinsics::ID, and one for SystemDictionary::WKID).

: int is the base type of the enum. I think that's part of the type's implementation so it shouldn't be exposed to the user of these types. If we litter enum class vmSymbolID : int; all over, when we decide to change the base type to something else, we would need to edit all such places.

Worse, if your forward declaration is wrong, like

enum class vmSymbolID : char;

the compiler won't detect this if your source file doesn't also include vmSymbols.hpp, so it could be dangerous.

I am okay with this suggestion.

I've moved the forward declarations into vmEnum.hpp in the latest version.

Ok, I can't find it. Why did you have to add this little file to replace the forward declarations? What was wrong with the forward declarations? There weren't that many.

vnkozlov

CI and JVMCI changes seem fine. I did not look in details on the rest.

openjdk · 2020-09-23T16:43:01Z

@iklam 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:

8253402: Convert vmSymbols::SID to enum class

Reviewed-by: kvn, coleenp, kbarrett, iveresov

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.

coleenp

Looks like a nice improvement to me.

coleenp · 2020-09-24T10:51:08Z

src/hotspot/share/ci/ciObjectFactory.hpp

@@ -29,6 +29,8 @@
 #include "ci/ciObject.hpp"
 #include "utilities/growableArray.hpp"

+enum class vmSymbolID : int;


No, don't move them to globalDefinitions.hpp, only have the forward declarations where they're used. So this is like a class forward declaration? neat.

coleenp · 2020-09-25T22:46:20Z

src/hotspot/share/utilities/vmEnums.hpp

+// complex header files that have the full definitions of these enums.
+
+enum JVMFlagsEnum : int;
+enum class vmSymbolID : int;


I don't like that you mixed two different enums that are used in different places here. Can you go back?

The intention is to put other enums like vmIntrinsics::ID and SystemDictionary::WKID here. It's going to be a small collection of "enums used by the VM that have complex definitions but you probably don't care".

vnkozlov

Good.

kimbarrett · 2020-09-27T11:09:30Z

src/hotspot/share/utilities/enumIterator.hpp

@@ -0,0 +1,93 @@
+/*


I think there are problems with this EnumIterator class. It was based in part on a prototype of mine, which I've been revisiting and revising, because I think it has problems. That prototype, in turn, was based on the existing WeakProcessorPhases::Iterator, which I think has some of the same failings. And I think this version expands on some of those problems. I don't yet have a full review, but below are some observations and issues. I'm working on an alternative.

A fundamental question is what style of "iterator" do we want.

(1) One style is self-contained; you create a single iterator which knows both the current position and the iteration limit, and step until a predicate (is_end() in the current code) is true.

(2) Another style is to have a pair of iterators, one designating the current position and the other designating the iteration limit. This is the style used by the C++ Standard Library.

Both my earlier prototype and the EnumIterator in this PR are 1-style but attempt (not necessarily very well, in my opinion) to also provide 2-style behavior. The point of that currently seems to be to support the new "range-based for" feature. (Said feature is currently not in the permitted list according to the Style Guide. I intentionally left it out because I think its utility is pretty strongly dependent on adopting 2-style iterators, which is not very well motivated without using the Standard Library.)

One requirement for an enum iterator (for me) is that it doesn't require a "fake" enumerator that designates the exclusive end of the range. The current proposal fails that test.

A problem with all of the variants is that they are trying to be both 1-style (providing is_end) and 2-style (providing being/end), with the result that they do neither well. This is especially true for the variant in the PR. I think part of the problem is that the begin/end functions don't belong to the iterator class; they should be part of a separate range class.

Aside: I think it is possible to provide iteration that doesn't assume sequential enumerators if one is willing to have some code duplication or has an enum that is x-macro based. While possibly an interesting exercise, I doubt that's worth pursuing. Just mentioning it in case anyone thinks this would actually useful.

I'm not certain how to proceed. Maybe this should be moved elsewhere as not yet ready to be a widely used "utility"? Or maybe go ahead with it with the intention of improving it?

Hi Kim, I have integrated the 2-style enumerator that you sent me off-line. Usage info (see enumIterator.hpp for details).

// Example (see vmSymbols.hpp/cpp) // // ENUMERATOR_RANGE(vmSymbolID, vmSymbolID::FIRST_SID, vmSymbolID::LAST_SID) // constexpr EnumRange<vmSymbolID> vmSymbolsRange; // using vmSymbolsIterator = EnumIterator<vmSymbolID>; // // /* Without range-based for, allowed */ // for (vmSymbolsIterator it = vmSymbolsRange.begin(); it != vmSymbolsRange.end(); ++it) { // vmSymbolID index = *it; .... // } // // /* With range-base for, not allowed by HotSpot coding style yet */ // for (vmSymbolID index : vmSymbolsRange) { // .... // }

I have rewritten all the iteration loops using the "Without range-based for" style. We can change to the "With range-base for" style when the HotSpot Coding Style Guide allows it.

openjdk · 2020-10-07T08:52:18Z

@iklam 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 8253402-convert-vmsymbols-sid-to-enum-class
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

kimbarrett · 2020-10-13T15:33:25Z

src/hotspot/share/classfile/vmSymbols.hpp

+    return (id >= FIRST_SID && id < SID_LIMIT);
+  }
+  static constexpr bool is_valid_id(vmSymbolID sid) {
+    return (static_cast<int>(sid) >= FIRST_SID && static_cast<int>(sid) < SID_LIMIT);


Why not just return is_valid_id(static_cast<int>(sid));

kimbarrett · 2020-10-13T15:35:45Z

src/hotspot/share/classfile/vmSymbols.hpp

-    VM_SYMBOLS_DO(VM_SYMBOL_IGNORE, VM_ALIAS_ENUM)
-    #undef VM_ALIAS_ENUM
+  static constexpr int number_of_symbols() {
+    return SID_LIMIT;


SID_LIMIT includes NO_SID, so this seems to be off-by-one.

I changed NO_SID to -1 so SID_LIMIT no longer includes NO_SID.

I needed to revert NO_SID to 0. There's an assert in vmSymbols.cpp for Symbol::_vm_symbols[NO_SID] == NULL. This is pre-existing code and I don't understand why, but I should leave it as. As you can see in the old code, NO_SID is also counted as "part of the symbols"

- static vmSymbols::SID vm_symbol_index[vmSymbols::SID_LIMIT]; + static vmSymbolID vm_symbol_index[vmSymbols::number_of_symbols()];

kimbarrett · 2020-10-13T15:42:41Z

src/hotspot/share/classfile/vmSymbols.hpp


-    FIRST_SID = NO_SID + 1
-  };
  enum {
    log2_SID_LIMIT = 11         // checked by an assert at start-up


[pre-existing, could be followup]
Why isn't this checked with a static assert right here?
SID_LIMIT <= (1 << log2_SID_LIMIT)
is (and always has been) compile-time checkable. And log2_SID_LIMIT should no longer be an enum. (Note that the value isn't currently constexpr-calculable because round_up_power_of_2 can't currently be constexpr.)

The existing code groups several asserts together. I think this makes the intention clearer. I'll leave it for now.

void vmSymbols::initialize(TRAPS) { assert(SID_LIMIT <= (1<<log2_SID_LIMIT), "must fit in this bitfield"); assert(SID_LIMIT*5 > (1<<log2_SID_LIMIT), "make the bitfield smaller, please"); assert(vmIntrinsics::FLAG_LIMIT <= (1 << vmIntrinsics::log2_FLAG_LIMIT), "must fit in this bitfield");

kimbarrett · 2020-10-13T15:55:25Z

src/hotspot/share/utilities/enumIterator.hpp

+  };
+
+template<typename T>
+class EnumIterationTraits : AllStatic {


A comment for this class might be helpful. Something like "A helper class for EnumIterator, computing some additional information the iterator uses, based on T and EnumeratorRange." The main point being that users of enum iteration don't need to think about this.

kimbarrett · 2020-10-13T15:56:51Z

src/hotspot/share/utilities/enumIterator.hpp

+// Don't bother trying to figure this out.
+//
+//
+// EnumIterationTraits -- defines the static range of all possible values of the enum.


This traits class isn't really interesting to users of this facility; it's more of an implementation detail. See my comment below on the definition. It's EnumeratorRange and (especially) ENUMERATOR_RANGE that are interesting to users.

kimbarrett · 2020-10-13T16:00:54Z

src/hotspot/share/utilities/enumIterator.hpp

+    assert(_value <= Traits::_end, "out of range");
+  }
+
+  // True if the enumerators designate the same value.


"True if the iterators designate the same enumeration value." (Sorry about that.) Similarly below for operator!=.

iklam · 2020-10-15T05:51:23Z

/integrate

openjdk · 2020-10-15T05:53:41Z

@iklam Pushed as commit 7e5eb49.

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

iklam added 2 commits September 20, 2020 23:44

8253402: Convert vmSymbols::SID to enum class

acc1101

removed unnecessary include

1a2eb44

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Sep 21, 2020

clean up whitespaces and removed useless comment

443cf52

openjdk bot removed the serviceability serviceability-dev@openjdk.org label Sep 21, 2020

iklam marked this pull request as ready for review September 21, 2020 07:16

openjdk bot added the rfr Pull request is ready for review label Sep 21, 2020

vnkozlov reviewed Sep 23, 2020

View reviewed changes

vnkozlov approved these changes Sep 23, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 23, 2020

coleenp approved these changes Sep 24, 2020

View reviewed changes

iklam added 3 commits September 25, 2020 12:53

Moved forward declaration of vmSymbolID to vmEnums.hpp

a8ad647

Merge branch 'master' into 8253402-convert-vmsymbols-sid-to-enum-class

0b1e8b2

more vmEnums.hpp fixes; fixed minimal VM build

1ad27fd

coleenp reviewed Sep 25, 2020

View reviewed changes

vnkozlov approved these changes Sep 26, 2020

View reviewed changes

kimbarrett suggested changes Sep 27, 2020

View reviewed changes

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Oct 7, 2020

iklam added 2 commits October 7, 2020 15:53

Merge master into 8253402-convert-vmsymbols-sid-to-enum-class

4c474b6

Use 2-style EnumIterator

9deb681

openjdk bot added the ready Pull request is ready to be integrated label Oct 10, 2020

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Oct 10, 2020

added missing #include <limits> from enumIterator.hpp

9ddca08

veresov approved these changes Oct 12, 2020

View reviewed changes

vnkozlov approved these changes Oct 12, 2020

View reviewed changes

kimbarrett suggested changes Oct 13, 2020

View reviewed changes

Addressed review comments by Kim Barrett

662ad8b

kimbarrett approved these changes Oct 14, 2020

View reviewed changes

iklam added 3 commits October 13, 2020 19:58

revert NO_SID to 0 due to assert(Symbol::_vm_symbols[NO_SID] == NULL)

1096cbf

Merge branch 'master' into 8253402-convert-vmsymbols-sid-to-enum-class

60178ea

Merge branch 'master' into 8253402-convert-vmsymbols-sid-to-enum-class

5e939ca

openjdk bot closed this Oct 15, 2020

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 Oct 15, 2020

iklam mentioned this pull request Nov 16, 2020

8256254: Convert vmIntrinsics::ID to enum class #1237

Closed

3 tasks

8253402: Convert vmSymbols::SID to enum class #276

8253402: Convert vmSymbols::SID to enum class #276

iklam commented Sep 21, 2020 •

edited by openjdk bot

bridgekeeper bot commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

iklam commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

mlbridge bot commented Sep 21, 2020 •

edited

vnkozlov Sep 23, 2020

iklam Sep 23, 2020

coleenp Sep 24, 2020

iklam Sep 24, 2020

vnkozlov Sep 25, 2020

iklam Sep 25, 2020

coleenp Sep 25, 2020 •

edited

vnkozlov left a comment

openjdk bot commented Sep 23, 2020 •

edited

coleenp left a comment

coleenp Sep 24, 2020

coleenp Sep 25, 2020

iklam Sep 25, 2020

vnkozlov left a comment

kimbarrett Sep 27, 2020

iklam Oct 10, 2020

openjdk bot commented Oct 7, 2020

kimbarrett Oct 13, 2020

kimbarrett Oct 13, 2020

iklam Oct 13, 2020

iklam Oct 14, 2020

kimbarrett Oct 13, 2020

iklam Oct 13, 2020

kimbarrett Oct 13, 2020

kimbarrett Oct 13, 2020

kimbarrett Oct 13, 2020

iklam commented Oct 15, 2020

openjdk bot commented Oct 15, 2020

8253402: Convert vmSymbols::SID to enum class #276

8253402: Convert vmSymbols::SID to enum class #276

Conversation

iklam commented Sep 21, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

iklam commented Sep 21, 2020

openjdk bot commented Sep 21, 2020

mlbridge bot commented Sep 21, 2020 • edited

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coleenp Sep 25, 2020 • edited

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

openjdk bot commented Sep 23, 2020 • edited

coleenp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam commented Oct 15, 2020

openjdk bot commented Oct 15, 2020

iklam commented Sep 21, 2020 •

edited by openjdk bot

mlbridge bot commented Sep 21, 2020 •

edited

coleenp Sep 25, 2020 •

edited

openjdk bot commented Sep 23, 2020 •

edited