8243208: Clean up JVMFlag implementation

[iklam]

The initial commit d85cbd6 is the same patch as 8243208-cleanup-jvmflag-impl.v02 published in hotspot-dev@openjdk.java.net.

The rest of the review will continue on GitHub. I will add new commits to respond to comments to the above e-mail.

---

Progress

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

Issue

• JDK-8243208: Clean up JVMFlag implementation

Reviewers

• David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 1b63ac6
• Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to bf90a37
• Gerard Ziemski (@gerard-ziemski - Committer) ⚠️ Review applies to 1b63ac6

Download

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

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

[mlbridge]

Webrevs

• 06: Full - Incremental (af373e4)
• 05: Full - Incremental (1b63ac6)
• 04: Full - Incremental (15547eb)
• 03: Full - Incremental (bf90a37)
• 02: Full - Incremental (1024628)
• 01: Full - Incremental (2e387eb)
• 00: Full (d85cbd6)

[dholmes-ora]

One query below and a couple of style nits. Otherwise LGTM!

[openjdk]

@iklam This change now passes all automated pre-integration checks. In addition to the automated checks, the change must also fulfill all project specific requirements

After integration, the commit message will be:

8243208: Clean up JVMFlag implementation

Reviewed-by: dholmes, coleenp, gziemski

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

There are currently no new commits on the master branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate 976acddeb5a8df1e868269787c023306aad3fe4a.

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

[iklam]

Re: Gerard Ziemski's comments on hotspot-dev@openjdk.java.net:

(1) The API JVMFlagLimit::get_constraint(flag) says “constraint”, but it returns JVMFlagLimit* using different names for the same thing - one calls it “limit", the other uses “constraint”.

JVMFlagLimit is used for both constraints and ranges. I updated the comments:

// A JVMFlagLimit is created for each JVMFlag that has a range() and/or constraint() in its declaration in
// the globals_xxx.hpp file.
//
// To query the range information of a JVMFlag:
//     JVMFlagLimit::get_range(JVMFlag*)
//     JVMFlagLimit::get_range_at(int flag_enum)
// If the given flag doesn't have a range, NULL is returned.
//
// To query the constraint information of a JVMFlag:
//     JVMFlagLimit::get_constraint(JVMFlag*)
//     JVMFlagLimit::get_constraint_at(int flag_enum)
// If the given flag doesn't have a constraint, NULL is returned.

(3) Isn’t there a better way to treat the special cases in "JVMFlagRangeList::print”, without explicitly checking for hardcoded function pointers? Or is it just not worth the effort, with only 2 such cases for now?

So far there are only 2 cases so I think the explicit checking may be enough. If we have more cases in the future we can have a more general solution.

(5) I'm not sure how others feel about it, but I think I personally would find it helpful for macros like “DEFINE_RANGE_CHECK” to see a comment with a concrete example of macro expansion for some flag, just to see what it works out to be.

I think this macro is fairly easy to read, so adding an expanded example may not have much benefit:

#define DEFINE_RANGE_CHECK(T)                                                            \
JVMFlag::Error JVMFlagRangeChecker::check_ ## T(T value, bool verbose) const {           \
  assert(exists(), "must be");                                                           \
  JVMFlagRange_ ## T range(_flag, _limit->as_ ## T()->min(), _limit->as_ ## T()->max()); \
  return range.check_ ## T(value, verbose);                                              \
}

In JDK-8081833, I plan to use templates to replaces the nearly identical code in jvmFlagRangeList.cpp/jvmFlagConstraintList.cpp, and these macros should go away.

(I fixed other minor nits found by Gerard.)

[dholmes-ora]

Updates seem okay. One query below.

[coleenp]

Can this function just call hash_code(s, strlen(s)) ?

[iklam]

strlen() is not constexpr so it can't be used here. I added my hand-rolled string_len function in bf90a37 so we can share a single copy of the hash calculation loop.

[coleenp]

Nice!

[gerard-ziemski]

I had the same question, cool.

[coleenp]

Looks good. I had a couple of minor comments.

In JDK-8081833, I plan to use templates to replaces the nearly identical code in
jvmFlagRangeList.cpp/jvmFlagConstraintList.cpp, and these macros should go away.
Good!

[coleenp]

Nice!

[iklam]

Re: Gerard Ziemski's comments on hotspot-dev@openjdk.java.net:

I made the changes in 15547eb

Gerard #6 Do we need to provide base of the enums 
(https://hg.openjdk.java.net/jdk/jdk/raw-file/tip/doc/hotspot-style.html#enum) 
in files:

a) open/src/hotspot/share/runtime/globals_extension.hpp
   43 typedef enum {
   44   ALL_FLAGS(FLAG_MEMBER_ENUM_DEVELOP,
   45             FLAG_MEMBER_ENUM_PD_DEVELOP,
   46             FLAG_MEMBER_ENUM_PRODUCT,
   47             FLAG_MEMBER_ENUM_PD_PRODUCT,
   48             FLAG_MEMBER_ENUM_NOTPRODUCT,
   49             IGNORE_RANGE,
   50             IGNORE_CONSTRAINT)
   51   NUM_JVMFlagsEnum
   52 } JVMFlagsEnum;

b) src/hotspot/share/runtime/flags/jvmFlagLimit.cpp
   46 enum JVMFlagConstraintsEnum {
   47   ALL_CONSTRAINTS(CONSTRAINT_ENUM_)
   48   NUM_JVMFlagConstraintsEnum
   49 };

I added enum ... : int as the base.

Gerard #7 Should these enums be replaced with ordinary constants 
(https://hg.openjdk.java.net/jdk/jdk/raw-file/tip/doc/hotspot-style.html#enum) 
in files:

a) open/src/hotspot/share/runtime/flags/jvmFlagLimit.hpp
   44 class JVMFlagLimit {
   45   enum {
   46     HAS_RANGE = 1,
   47     HAS_CONSTRAINT = 2
   48   };

Changed to static constexpr int HAS_RANGE = 1, etc.

Gerard #8 Can we add a description to 
"src/hotspot/share/runtime/flags/jvmFlagLookup.hpp” on how “constexpr" 
makes this hash table possible at the compile time?

I added

// This is a hashtable that maps from (const char*) to (JVMFlag*) to speed up
// the processing of JVM command-line argument at runtime.
//
// With constexpr, this table is generated at C++ compile time so there's
// no set up cost at runtime.
class JVMFlagLookup { ...

Gerard #9 Shouldn’t the code in JVMFlagLookup::JVMFlagLookup() initializing the 
internal tables be more like:

constexpr JVMFlagLookup::JVMFlagLookup() : _buckets(), _table(), _hashes() {
   for (int i = 0; i < NUM_BUCKETS; i++) {
     _buckets[i] = -1;
   }
   for (int i = 0; i < NUM_JVMFlagsEnum; i++) {
     _hashes[i] = 0;
     _table[i] = -1;
   }

There's no need to initialize _hashes and _table in a loop. Each of the elements in these two arrays are individually initialized by the expansion of this macro:

#define DO_HASH(flag_enum, flag_name) {          \
  unsigned int hash = hash_code(flag_name);      \
  int bucket_index = (int)(hash % NUM_BUCKETS);  \
  _hashes[flag_enum] = (u2)(hash);               \
  _table[flag_enum] = _buckets[bucket_index];    \
  _buckets[bucket_index] = (short)flag_enum;     \
}
...
  ALL_FLAGS(DO_FLAG,
            DO_FLAG,
            DO_FLAG,
            DO_FLAG,
            DO_FLAG,
            IGNORE_RANGE,
            IGNORE_CONSTRAINT)

There was a bug in my previous version that unnecessarily initialized _hashes[i] = 0 in the loop. I've removed it.

Gerard #10 “flagTable_verify_constexpr" and “flagTable”, in 
"src/hotspot/share/runtime/flags/jvmFlag.cpp”, are almost identical - 
can we find a way to create just one table (of the “constexpr” type)?

flagTable_verify_constexpr[] is needed for build-time checking only, and cannot be combined with flagTable[]. I've updated the comments to clarify:

// We want flagTable[] to be completely initialized at C++ compilation time, which requires
// that all arguments passed to JVMFlag() constructors to be constexpr. The following line
// checks for this this -- if any non-constexpr arguments are passed, the C++ compiler will
// generate an error.
//
// constexpr implies internal linkage. This means the flagTable_verify_constexpr[] variable
// will not be included in jvmFlag.o, so there's no footprint cost for having this variable.
//
// Note that we cannot declare flagTable[] as constexpr because JVMFlag::_flags is modified
// at runtime.
constexpr JVMFlag flagTable_verify_constexpr[] = { MATERIALIZE_ALL_FLAGS };

[mlbridge]

Mailing list message from David Holmes on shenandoah-dev:

Hi Ioi,

On 10/09/2020 1:41 am, Ioi Lam wrote:

On Wed, 9 Sep 2020 14:06:05 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

src/hotspot/share/runtime/flags/jvmFlag.cpp line 266:

264:
265: bool JVMFlag::is_writeable() const {
266: return is_manageable();

Unclear why is_writeable() reduces to is_manageable(). Aren't all product flags writeable? And aren't all develop flags
writeable in non-product builds?

All flags shouldn't be also writeable. Interpreter code generation depends on static value of some flags, for instance.

is_writeable() doesn't mean "modifiable". The meaning is

// manageable flags are writeable external product flags.
// They are dynamically writeable through the JDK management interface
// (com.sun.management.HotSpotDiagnosticMXBean API) and also through JConsole.

See also
[writeableFlags.cpp](https://github.com/openjdk/jdk/blob/433394203dce55db95caefcb57bac9ec114ebc47/src/hotspot/share/services/writeableFlags.cpp#L283).

There used to be `product_rw` flags, which were "writeable" but not "manageable". However, there aren't any of them
anymore. So I have deleted product_rw() from the comments.
/**** removed comments ****/
// product_rw flags are writeable internal product flags.
// They are like "manageable" flags but for internal/private use.
// The list of product_rw flags are internal/private flags which
// may be changed/removed in a future release. It can be set
// through the management interface to get/set value
// when the name of flag is supplied.
//
// A flag can be made as "product_rw" only if
// - the VM implementation supports dynamic setting of the flag.
// This implies that the VM must *always* query the flag variable
// and not reuse state related to the flag state at any given time.
//
// Note that when there is a need to support develop flags to be writeable,
// it can be done in the same way as product_rw.

I also removed the implementation of product_rw(). Line 269 is part of the product_rw() removal.

I considered adding support for product_rw() in the new flags implementation. It would be fairly trivial (similar to
how MANAGEABLE is supported) but I am hesitant to do it because:

- No one uses it currently
- There are no test cases

I don't want to have an untested implementation which someone might accidently use in the future without any testing.
If anyone indeed finds producy_rw useful, they can easily implement the functionality and can write tests for it at the
same time.

Yes sorry for the noise. Obviously most flags are not writeable - their
value has to be fixed at startup. Doh! I was confused by your earlier
webrev which showed "|| is_product()" and which seemed "obviously"
correct. :)

I'm somewhat surprised that none of our product or develop flags are
writeable any more. It used to be some Print/Trace flags that were
writeable and they have been replaced by UL. I can imagine wanting to
dynamically enable/disable diagnostic flags though ...

I wonder if "writeable" should be an attribute like
diagnostic/experimental, so that manageable flags are just writeable
product flags? That would be a future RFE of course (if appropriate).

Thanks,
David

[iklam]

/integrate

[openjdk]

@iklam Pushed as commit 5144190.

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

[mlbridge]

Mailing list message from Lindenmaier, Goetz on hotspot-dev:

Hi,

Since this change was pushed, we see the following build error on Windows:
.../src/hotspot/share/runtime/flags/jvmFlagLookup.cpp(54): error C2220: warning treated as error - no 'object' file generated
.../src/hotspot/share/runtime/flags/jvmFlagLookup.cpp(54): warning C4307: '*': integral constant overflow

We are compiling with Microsoft Visual Studio 2017.

(Is it the right way to write a mail to bring this up, or
should I comment the PullRequest or the like?)

Best regards,
Goetz

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

Hi Goetz,

On 14/09/2020 4:13 pm, Lindenmaier, Goetz wrote:

Hi,

Since this change was pushed, we see the following build error on Windows:
.../src/hotspot/share/runtime/flags/jvmFlagLookup.cpp(54): error C2220: warning treated as error - no 'object' file generated
.../src/hotspot/share/runtime/flags/jvmFlagLookup.cpp(54): warning C4307: '*': integral constant overflow

We are compiling with Microsoft Visual Studio 2017.

(Is it the right way to write a mail to bring this up, or
should I comment the PullRequest or the like?)

You can write a mail or just file a bug. :)

Is this 32-bit Windows? We have not seen any issue in our 64-bit builds.

Thanks,
David

Best regards,
Goetz

[mlbridge]

Mailing list message from Lindenmaier, Goetz on hotspot-dev:

Hi David,

It fails in the 32 and 64-bit build with the same message.

Best regards,
Goetz

-----Original Message-----
From: David Holmes <david.holmes at oracle.com>
Sent: Monday, September 14, 2020 8:45 AM
To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Ioi Lam
<ioi.lam at oracle.com>; hotspot-dev at openjdk.java.net; shenandoah-
dev at openjdk.java.net
Subject: Re: Integrated: 8243208: Clean up JVMFlag implementation

Hi Goetz,

On 14/09/2020 4:13 pm, Lindenmaier, Goetz wrote:

Hi,

Since this change was pushed, we see the following build error on Windows:
.../src/hotspot/share/runtime/flags/jvmFlagLookup.cpp(54): error C2220:
warning treated as error - no 'object' file generated
.../src/hotspot/share/runtime/flags/jvmFlagLookup.cpp(54): warning
C4307: '*': integral constant overflow

We are compiling with Microsoft Visual Studio 2017.

(Is it the right way to write a mail to bring this up, or
should I comment the PullRequest or the like?)

You can write a mail or just file a bug. :)

Is this 32-bit Windows? We have not seen any issue in our 64-bit builds.

Thanks,
David

Best regards,
Goetz

-----Original Message-----
From: hotspot-dev <hotspot-dev-retn at openjdk.java.net> On Behalf Of
Ioi
Lam
Sent: Friday, September 11, 2020 6:08 AM
To: hotspot-dev at openjdk.java.net; shenandoah-dev at openjdk.java.net
Subject: Integrated: 8243208: Clean up JVMFlag implementation

On Tue, 8 Sep 2020 18:33:41 GMT, Ioi Lam <iklam at openjdk.org> wrote:

The initial commit

[d85cbd6](https://github.com/openjdk/jdk/commit/d85cbd638f5b87c80eee

[mlbridge]

Mailing list message from David Holmes on hotspot-dev:

On 14/09/2020 5:02 pm, Lindenmaier, Goetz wrote:

Hi David,

It fails in the 32 and 64-bit build with the same message.

We are building with VS2019, so may be a VS bug - possibly constExpr
related. AFAICS line 54 is simply:

constexpr JVMFlagLookup _flag_lookup_table;

David

Best regards,
Goetz

-----Original Message-----
From: David Holmes <david.holmes at oracle.com>
Sent: Monday, September 14, 2020 8:45 AM
To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Ioi Lam
<ioi.lam at oracle.com>; hotspot-dev at openjdk.java.net; shenandoah-
dev at openjdk.java.net
Subject: Re: Integrated: 8243208: Clean up JVMFlag implementation

Hi Goetz,

On 14/09/2020 4:13 pm, Lindenmaier, Goetz wrote:

Hi,

Since this change was pushed, we see the following build error on Windows:
.../src/hotspot/share/runtime/flags/jvmFlagLookup.cpp(54): error C2220:
warning treated as error - no 'object' file generated
.../src/hotspot/share/runtime/flags/jvmFlagLookup.cpp(54): warning
C4307: '*': integral constant overflow

We are compiling with Microsoft Visual Studio 2017.

(Is it the right way to write a mail to bring this up, or
should I comment the PullRequest or the like?)

You can write a mail or just file a bug. :)

Is this 32-bit Windows? We have not seen any issue in our 64-bit builds.

Thanks,
David

Best regards,
Goetz

-----Original Message-----
From: hotspot-dev <hotspot-dev-retn at openjdk.java.net> On Behalf Of
Ioi
Lam
Sent: Friday, September 11, 2020 6:08 AM
To: hotspot-dev at openjdk.java.net; shenandoah-dev at openjdk.java.net
Subject: Integrated: 8243208: Clean up JVMFlag implementation

On Tue, 8 Sep 2020 18:33:41 GMT, Ioi Lam <iklam at openjdk.org> wrote:

The initial commit

[d85cbd6](https://github.com/openjdk/jdk/commit/d85cbd638f5b87c80eee

[iklam]

Maybe it's complaining about this "*" in jvmFlagLookup.hpp? How about this:

  static constexpr unsigned int hash_code(const char* s, size_t len) {
    unsigned int h = 0;
    while (len -- > 0) {
-     h = 31*h + (unsigned int) *s;
+     h = ((unsigned int)31)*h + (unsigned int) *s;
      s++;
    }
    return h;

[shipilev]

Seeing Windows build failures on some of my Windows build nodes as well. Filed https://bugs.openjdk.java.net/browse/JDK-8253089, please discuss there.