8255285: Move JVMFlag origins into a new enum JVMFlagOrigin

[iklam]

Many JVM function take an JVMFlag::Flags parameter to indicate the origin of the flag -- i.e., "who is setting this flag". E.g., in arguments.hpp:

static bool parse_argument(const char* arg, JVMFlag::Flags origin);

However, JVMFlag::Flags contains many other bits that are unrelated to the origin. We should add a new enum JVMFlagOrigin that has only the valid values for the origin. This makes it possible to do more type-safety checks at C++ compilation time.

This patch also renamed the confusing bit JVMFlag::ORIG_COMMAND_LINE to WAS_SET_IN_COMMAND_LINE and added documentation, so that it won't be confused with JVMFlagOrigin::COMMAND_LINE.

---

Progress

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

Issue

• JDK-8255285: Move JVMFlag origins into a new enum JVMFlagOrigin

Reviewers

• David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 53fed1b
• Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to b1d5380

Download

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

[bridgekeeper]

👋 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]

@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 pull request command.

[iklam]

/label remove serviceability

[openjdk]

@iklam
The serviceability label was successfully removed.

[mlbridge]

Webrevs

• 03: Full - Incremental (2e0552d)
• 02: Full - Incremental (b1d5380)
• 01: Full - Incremental (53fed1b)
• 00: Full (ab81483)

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

Hi Ioi,

On 23/10/2020 4:52 pm, Ioi Lam wrote:

Many JVM function take an `JVMFlag::Flags` parameter to indicate the origin of the flag -- i.e., "who is setting this flag". E.g., in arguments.hpp:

static bool parse_argument(const char* arg, JVMFlag::Flags origin);

However, `JVMFlag::Flags` contains many other bits that are unrelated to the origin. We should add a new enum `JVMFlagOrigin` that has only the valid values for the origin. This makes it possible to do more type-safety checks at C++ compilation time.

This patch also renamed the confusing bit `JVMFlag::ORIG_COMMAND_LINE` to `WAS_SET_IN_COMMAND_LINE` and added documentation, so that it won't be confused with `JVMFlagOrigin::COMMAND_LINE`.

I'm still confused :) Why are we reporting "command line" for a flag
that was ergonomically set, or vice-versa? Surely a flag is either set
via the command-line or via ergonomics but not both ?? I was under the
assumption that ergonomics should not touch a flag explicitly set on the
command-line as that defeats the purpose of setting it.

enum class JVMFlagOrigin

Why not define this as

enum class Origin

inside class JVMFlag, so that it is then referred to as JVMFlag::Origin?

static const JVMFlagOrigin DEFAULT = JVMFlagOrigin::DEFAULT;

Why is this needed?? To avoid re-typing JVMFlagOrigin?

Cheers,
David
-----

[iklam]

Mailing list message from David Holmes on hotspot-dev:

Hi Ioi,

On 23/10/2020 4:52 pm, Ioi Lam wrote:

This patch also renamed the confusing bit JVMFlag::ORIG_COMMAND_LINE to WAS_SET_IN_COMMAND_LINE and added documentation, so that it won't be confused with JVMFlagOrigin::COMMAND_LINE.

I'm still confused :) Why are we reporting "command line" for a flag
that was ergonomically set, or vice-versa? Surely a flag is either set
via the command-line or via ergonomics but not both ??

We have code like this:

void JVMFlag::print_origin(outputStream* st, unsigned int width) const {
    case JVMFlagOrigin::ERGONOMIC:
      if (_flags & WAS_SET_IN_COMMAND_LINE) {
        st->print("command line, ");
      }
      st->print("ergonomic"); break;

So if FLAG_SET_ERGO changes a flag that was specified in the command-line, we will print out "command line, ergonomic".

I was under the
assumption that ergonomics should not touch a flag explicitly set on the
command-line as that defeats the purpose of setting it.

I have no idea why this is the case. Maybe ergonomics is allowed to "fine tune" user-specified values? Anyway, if we want to change this, we should do it in a separate RFE.

enum class JVMFlagOrigin

Why not define this as

enum class Origin

inside class JVMFlag, so that it is then referred to as JVMFlag::Origin?

The reason is to allow JVMFlagOrigin to be used in a forward declaration without including jvmFlag.hpp. See vmEnums.hpp.

A nested enum like JVMFlag::Origin cannot be forward-declared.

static const JVMFlagOrigin DEFAULT = JVMFlagOrigin::DEFAULT;

Why is this needed?? To avoid re-typing JVMFlagOrigin?

Yeah, but I removed this in the latest version 53fed1b

[openjdk]

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

8255285: Move JVMFlag origins into a new enum JVMFlagOrigin

Reviewed-by: dholmes, redestad

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]

Mailing list message from David Holmes on hotspot-dev:

Hi Ioi,

On 27/10/2020 8:32 am, Ioi Lam wrote:

On Fri, 23 Oct 2020 06:33:06 GMT, Ioi Lam <iklam at openjdk.org> wrote:

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-dev](mailto:hotspot-dev at openjdk.java.net):_

Hi Ioi,

On 23/10/2020 4:52 pm, Ioi Lam wrote:

This patch also renamed the confusing bit `JVMFlag::ORIG_COMMAND_LINE` to `WAS_SET_IN_COMMAND_LINE` and added documentation, so that it won't be confused with `JVMFlagOrigin::COMMAND_LINE`.

I'm still confused :) Why are we reporting "command line" for a flag
that was ergonomically set, or vice-versa? Surely a flag is either set
via the command-line or via ergonomics but not both ??

We have code like this:

void JVMFlag::print_origin(outputStream* st, unsigned int width) const {
case JVMFlagOrigin::ERGONOMIC:
if (_flags & WAS_SET_IN_COMMAND_LINE) {
st->print("command line, ");
}
st->print("ergonomic"); break;

So if FLAG_SET_ERGO changes a flag that was specified in the command-line, we will print out "command line, ergonomic".

I was under the
assumption that ergonomics should not touch a flag explicitly set on the
command-line as that defeats the purpose of setting it.

I have no idea why this is the case. Maybe ergonomics is allowed to "fine tune" user-specified values? Anyway, if we want to change this, we should do it in a separate RFE.

Yes separate RFE. This might be necessary/desirable but at a minimum it
should be clearly documented when ergonomics can override an explicit
user setting.

enum class JVMFlagOrigin

Why not define this as

enum class Origin

inside class JVMFlag, so that it is then referred to as JVMFlag::Origin?

The reason is to allow `JVMFlagOrigin` to be used in a forward declaration without including jvmFlag.hpp. See vmEnums.hpp.

A nested enum like `JVMFlag::Origin` cannot be forward-declared.

That is a pity. I'm not sure I agree with the overall approach of making
enums all top-level just to minimise the number of includes needed. Code
structure is more important to me than shaving a few seconds off build
times.

static const JVMFlagOrigin DEFAULT = JVMFlagOrigin::DEFAULT;

Why is this needed?? To avoid re-typing JVMFlagOrigin?

Yeah, but I removed this in the latest version [53fed1b](https://github.com//pull/823/commits/53fed1b00784763c873bf81475430e280f06d72c)

Okay.

Thanks,
David

[iklam]

/integrate

[openjdk]

@iklam Pushed as commit 1a89d68.

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