RFR(L) 8243208 Clean up JVMFlag implementation
gerard ziemski
gerard.ziemski at oracle.com
Thu Sep 3 16:58:29 UTC 2020
hi Ioi,
Very nice work!
I reviewed most of the files already (they look good), but the core
implementation is taking me more time, so for now I’m going to share
what I have so far on file
src/hotspot/share/runtime/flags/jvmFlagRangeList.cpp:
#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”. Maybe instead of
282 JVMFlagRangeChecker range = JVMFlagRangeList::find(flag);
283 if (range.exists()) {
284 range.print(st);
285 } else {
286 const JVMFlagLimit* limit = JVMFlagLimit::get_constraint(flag);
287 if (limit != NULL) {
we could have this:
282 JVMFlagRangeChecker range = JVMFlagRangeList::find(flag);
283 if (range.exists()) {
284 range.print(st);
285 } else {
286 const JVMFlagLimit* limit = JVMFlagLimit::find(flag);
287 if (limit != NULL) {
or alternatively rename “JVMFlagLimit” to “JVMFlagConstraint” ?
#2 Comment:
290 // Two special cases who's range lower limit is an os::
function call and cannot
should be:
290 // Two special cases whose range lower limit is an os::
function call and cannot
#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?
#4 Inconsistent white space usage in:
245 #define DEFINE_RANGE_CHECK(T) \
246 JVMFlag::Error JVMFlagRangeChecker::check_ ## T(T value, bool
verbose) const { \
247 assert(exists(), "must
be"); \
248 JVMFlagRange_ ## T range(_flag, _limit->as_##T()->min(),
_limit->as_ ## T()->max()); \
249 return range.check_ ## T(value,
verbose); \
For example, at line 248 we have "_limit->as_##T()->min()” (no spaces
around ##) then "_limit->as_ ## T()->max()"
#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’ll have more feedback soon…
cheers
On 9/2/20 1:37 PM, Ioi Lam wrote:
> I have an updated version (with fewer changes!) as suggested to me
> Stefan Karlsson offline:
>
> http://cr.openjdk.java.net/~iklam/jdk16/8243208-cleanup-jvmflag-impl.v02/
>
> [1] More consistent white spaces in the macro definitions
>
> [2] Reverted the renaming of JVMFlag::Flags. This renaming is not
> really needed by
> the cleanup. Doing it later will avoid unnecessary clutter in the
> changeset.
>
> Also Stefan suggested renaming to JVMFlag::Attributes (instead of
> ::Attrs).
>
> [3] Removed some TODO comments as they are already captured in JBS
> issues.
>
> Thanks
> - Ioi
>
>
>
> On 9/1/20 6:29 PM, 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
>
More information about the hotspot-dev
mailing list