RFR (M): 8078556: Runtime: implement ranges (optionally constraints) for those flags that have them missing

[Dmitry Dmitriev]

Hi Gerard,

Two additional comments:
1) Coleen comment about Shared* flags remind me that we need take into 
account flags relationship and in case of Shared*Size flags we need to 
add "-Xshare:dump" when testing. Can you please add following to the 
'switch' statement in addNameDependency 
method(test/runtime/CommandLine/OptionsValidation/common/optionsvalidation/JVMOptionsUtils.java 
module):
case "SharedBaseAddress":
case "SharedMiscCodeSize":
case "SharedMiscDataSize":
case "SharedReadOnlySize":
case "SharedReadWriteSize":
case "SharedSymbolTableBucketSize":
       option.addPrepend("-Xshare:dump");
       break;

I will also look into other dependencies.

2) I noticed that you remove processing of  "XX:MaxDirectMemorySize" 
flag from src/share/vm/runtime/arguments.cpp. Range check is covered by 
new framework, but in this case we also lose ability to specify 
"XX:MaxDirectMemorySize" with following values: "10m", "1g" and etc. 
This probably can affect users. I prefer to leave that processing, but 
change error message to the standard error message: "Improperly 
specified VM option 'MaxDirectMemorySize=<value>'

Thank you,
Dmitry

On 30.09.2015 3:25, Coleen Phillimore wrote:
>
> Coming up with ranges for these flags and verifying them is a 
> significant body of work.  This looks really good.
>
> I have a couple comments.
>
> ----
>
> These options Shared*Size should probably not have a minimum size of 
> zero.   What happens if you run -Xshare:dump -XX:+SharedMiscSize=0, as 
> an example?
>
> I think it would be good to have definitions of 
> DEFAULT_SHARED_READ_WRITE_SIZE = *NOT_LP64(12*M) LP64_ONLY(16*M),* or 
> as constant values above in the file and have the ranges be min = 
> default - 1M and max = default + 1M or something like that.
>
> Also SharedBaseAddress = 0 shouldn't be allowed, it should be at least 
> >= HeapBaseMinAddress.
>
> ----
>
> I don't see any callers of 
> Arguments::get_default_heap_base_min_adderss in your patch.
>
> ----
>
> I don't have a problem with TraceRedefineClasses max range being 
> 0x80000000 even though the value is unused.  It won't cause any 
> problems if it is set.
>
> Thanks,
> Coleen
>
>
> On 9/18/15 12:49 PM, gerard ziemski wrote:
>>
>> hi all,
>>
>> Please review this change that implements ranges and constraint for 
>> runtime flags.
>>
>> The purpose of the ranges is to define valid ranges, that are 
>> guaranteed to be accepted and work when instantiating a VM. The VM is 
>> allowed to exit with an exception due to out of OS resources (like 
>> memory), but it may not assert or prematurely exit with an error that 
>> produces hs_err and/or dump core.
>>
>> The following flags had their ranges defined by the the domain experts:
>>
>> AttachListenerTimeout: Staffan Larsen
>> BiasedLockingBulkRebiasThreshold: Marcus Larsson
>> BiasedLockingBulkRevokeThreshold: Marcus Larsson
>> BiasedLockingDecayTime: Marcus Larsson
>> BiasedLockingStartupDelay: Marcus Larsson
>> SharedBaseAddress: Yumin Qi
>> SharedMiscCodeSize: Yumin Qi
>> SharedMiscDataSize: Yumin Qi
>> SharedReadOnlySize: Yumin Qi
>> SharedReadWriteSize: Yumin Qi
>> SharedSymbolTableBucketSize: Yumin Qi
>> StackShadowPages: Coleen Phillimore, Frederic Parain
>>
>> For the rest of the flags, I looked at the existing code that was 
>> using them and also did local tests:
>>
>> DeferPollingPageLoopCount
>> DeferThrSuspendLoopCount
>> GuaranteedSafepointInterval
>> HeapBaseMinAddress
>> InitialBootClassLoaderMetaspaceSize
>> JavaPriority10_To_OSPriority
>> JavaPriority1_To_OSPriority
>> JavaPriority2_To_OSPriority
>> JavaPriority3_To_OSPriority
>> JavaPriority4_To_OSPriority
>> JavaPriority5_To_OSPriority
>> JavaPriority6_To_OSPriority
>> JavaPriority7_To_OSPriority
>> JavaPriority8_To_OSPriority
>> JavaPriority9_To_OSPriority
>> MallocMaxTestWords
>> MallocVerifyInterval
>> MallocVerifyStart
>> MaxDirectMemorySize
>> MaxJNILocalCapacity
>> MaxJavaStackTraceDepth
>> MaxRAM
>> MinPassesBeforeFlush
>> PerfDataMemorySize
>> PerfDataSamplingInterval
>> PerfMaxStringConstLength
>> PrintSafepointStatisticsCount
>> PrintSafepointStatisticsTimeout
>> ProfileIntervalsTicks
>> RTMRetryCount
>> SafepointSpinBeforeYield
>> SafepointTimeoutDelay
>> SelfDestructTimer
>> SuspendRetryCount
>> SuspendRetryDelay
>> ThreadSafetyMargin
>> ThreadStackSize
>> TraceRedefineClasses
>> VMThreadPriority
>> VMThreadStackSize
>>
>> The change passes “JPRT -testset hotspot”, “RBT 
>> hotspot/test/runtime/CommandLine” and “RBT testlist,noncolo.testlist 
>> --add-tonga-keyword quick”
>>
>> References:
>>
>>  bugid: https://bugs.openjdk.java.net/browse/JDK-8078556
>> webrev: http://cr.openjdk.java.net/~gziemski/8078556_rev0/
>>
>> Followup issues:
>>
>>  https://bugs.openjdk.java.net/browse/JDK-8133564 "Runtime - 2nd 
>> followup to JDK-8059557"
>>  https://bugs.openjdk.java.net/browse/JDK-8136766 "Enable 
>> ThreadStackSize range test”
>>  https://bugs.openjdk.java.net/browse/JDK-8134691 
>> "CommandLineFlagConstraint::AtParse is not useful and not what we need"
>>
>> hg_stat:
>>
>>  src/cpu/aarch64/vm/globals_aarch64.hpp |  10 ++-
>>  src/cpu/sparc/vm/globals_sparc.hpp |  16 ++++--
>>  src/cpu/x86/vm/globals_x86.hpp |  18 +++++-
>>  src/cpu/zero/vm/globals_zero.hpp |  11 +++-
>>  src/os/aix/vm/globals_aix.hpp |   7 +-
>>  src/share/vm/runtime/arguments.cpp |  16 +-----
>>  src/share/vm/runtime/arguments.hpp |   2 +
>>  src/share/vm/runtime/commandLineFlagConstraintList.cpp |   2 +-
>>  src/share/vm/runtime/commandLineFlagConstraintList.hpp |   4 +-
>>  src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp | 73 
>> ++++++++++++++++++++++++++++++-
>>  src/share/vm/runtime/commandLineFlagConstraintsRuntime.hpp | 9 +++
>>  src/share/vm/runtime/commandLineFlagRangeList.cpp |   3 +-
>>  src/share/vm/runtime/globals.hpp |  67 +++++++++++++++++++++++++--
>>  src/share/vm/runtime/vmThread.cpp |   2 +-
>>  test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java |   
>> 7 ++
>>  15 files changed, 202 insertions(+), 45 deletions(-)
>>
>>
>> Thank you.
>>
>