RFR(L) 8243208 Clean up JVMFlag implementation

[Ioi Lam]

Hi Stefan,

Thanks for your comments & patch!

On 9/2/20 2:09 AM, Stefan Karlsson wrote:
> Hi Ioi,
>
> I did an initial scan over the change, to see what it looked like from 
> the GC perspective, but didn't actually look at the code in detail.
>
> Would you mind making these whitespace changes to make the code a bit 
> more clear and consistent:
> https://cr.openjdk.java.net/~stefank/8243208/review/webrev.01/
>
Fixed.

> Regarding the rename Flags to Attrs. I think this is one of those 
> things that would be nice to get separated out into a preparation 
> patch. Also, I'm not sure about the name Attrs. It's obscure, and even 
> our guidelines discourage introducing abbreviated names. Could we 
> rename this to Attributes?
>

I reverted Attrs/attrs back to Flags/flags. This eliminated a lot of 
deltas in the code so hopefully will make it a little easier to review :-)


I think I'll can do the renaming of Flags -> Attributes after the 
initial push, as it's not really needed in the main clean up.

> I think this shouldn't be pushed. It's fine during the review, but I 
> don't think we should leave these breadcrumbs in the code. It should 
> be enough that it has been logged as an RFE.
>
> // TODO:
> // JVMFlagConstraintChecker is just temporary code to bridge 
> JVMFlagLimit with the existing
> // JVMFlagConstraint_XXX classes. All of these will be gone when we 
> templatize the
> // range/constraint checks (JDK-8081833).
>
Removed.

http://cr.openjdk.java.net/~iklam/jdk16/8243208-cleanup-jvmflag-impl.v02/

Thanks
- Ioi

> Thanks,
> StefanK
>
> On 2020-09-02 03:29, Ioi Lam wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8243208
>> http://cr.openjdk.java.net/~iklam/jdk16/8243208-cleanup-jvmflag-impl.v01/ 
>>
>> https://wiki.openjdk.java.net/display/HotSpot/Hotspot+Command-line+Flags+Clean+Up+-+Design+Proposal+for+JDK-8243208 
>>
>>
>>
>>
>> Please review this major clean up of HotSpot command-line flags. The 
>> goals are:
>>
>>      + Reduce the complexity of the implementation
>>      + Improvement footprint/performance (with C++ constexpr)
>>      + Allow more flexibility in flag declaration (see also JDK-7123237)
>>
>>
>>
>> **** Notable interface changes:
>>
>>      I tried to make the changes non-intrusive. Most HotSpot developers
>>      should not be affected at all, unless you touch the following 
>> areas:
>>
>> [1] The declaration of diagnostic/experimental/manageable flags is 
>> changed.
>>      See globals.hpp for examples:
>>
>>      OLD: experimental(intx, hashCode, 5, "..docs...")
>>      NEW: product(intx, hashCode, 5, EXPERIMENTAL, "..docs...")
>>
>>      The reason is to support more flexible combinations. Before,
>>      all experimental flags must be "product". Now they can be "develop"
>>      as well.
>>
>> [2] Declaration of constraint functions is changed to use a macro.
>>      See jvmFlagConstraintsRuntime.hpp
>>
>> [3] The confusing type JVMFlag::Flags is renamed to JVMFlag::Attrs to be
>>      clear it's about "attributes of the flags" and not "the flags
>>      themselves".
>>
>> [4] The min/max of all ranges must be compile-time constants. We had
>>      two cases where the min/max was specified via os::xxx function
>>      calls. These are converted to constraint functions:
>>
>>      OLD:
>>      product_pd(uintx, InitialCodeCacheSize, "...") \
>>              range(os::vm_page_size(), max_uintx)
>>
>>      product(size_t, NUMAInterleaveGranularity, 2*M, "...")  \
>>              range(os::vm_allocation_granularity(), \
>>                    NOT_LP64(2*G) LP64_ONLY(8192*G)) \
>>
>>      NEW:
>>      product_pd(uintx, InitialCodeCacheSize, "...") \
>>              constraint(VMPageSizeConstraintFunc, AtParse)
>>
>>      product(size_t, NUMAInterleaveGranularity, 2*M, "...")  \
>> constraint(NUMAInterleaveGranularityConstraintFunc, \
>>                          AtParse)
>>
>>
>>
>> **** Notable implementation changes:
>>
>> [1] C++ constexpr is used extensively for build-time initialization of
>>      various tables.
>>
>> [2] All XXX_FLAGS() macros now consistent take 7 arguments. Application
>>      of these macros in jvmFlag.cpp have been greatly simplified.
>>
>>      This also makes it possible for further modularization in the 
>> future
>>      (see JDK-8243205).
>>
>>      example:
>>
>>      #define RUNTIME_FLAGS(develop, \
>>                            develop_pd, \
>>                            product, \
>>                            product_pd, \
>>                            notproduct, \
>>                            range, \
>>                            constraint) \
>>
>> [3] When processing the command-line arguments, the JVM flags are now
>>      searched using a build-time generated hashtable. See 
>> jvmFlagLookup.cpp
>>
>> [4] Tables for range/constraint check used to be dynamically constructed
>>      and linearly searched. Now they are build-time generated, with O(1)
>>      access time (a simple array index).
>>
>> For detailed discussion on the new design, please see
>> https://wiki.openjdk.java.net/display/HotSpot/Hotspot+Command-line+Flags+Clean+Up+-+Design+Proposal+for+JDK-8243208 
>>
>>
>>
>>
>> **** Next steps
>>
>>      I tried to keep this changeset small. So some changes are deferred:
>>
>> [1] In an separate RFE (JDK-8081833) I will use C++ templates
>>      to consolidate the large amount of duplicated code in jvmFlag.cpp,
>>      jvmFlagRangeList.cpp and jvmFlagConstraintList.cpp.
>>
>> [2] There are archaic test cases that assume that experimental flags
>>      must be product flags. See JVMFlag::JVMFlag() constructor in 
>> jvmFlag.cpp.
>>      I'll relax this in JDK-7123237 and update the test cases.
>>
>>
>> Thanks
>> - Ioi