RFR: 8243208: Clean up JVMFlag implementation [v4]

[Ioi Lam]

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

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Refactored JVMFlagLookup::hash_code(); Cleaned up LimitGetter::no_limit()
>
> Marked as reviewed by coleenp (Reviewer).

Re: [Gerard Ziemski's comments on
hotspot-dev at openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-dev/2020-September/043019.html):

I made the changes in [15547eb](https://github.com/openjdk/jdk/pull/82/commits/15547ebe703fcc3a4ce154f2a01f45b756051c26)

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

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

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