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

gerard ziemski gerard.ziemski at oracle.com
Mon Sep 28 18:12:43 UTC 2015 

Thank you Dmitry for a very thorough review:


On 09/25/2015 08:57 AM, Dmitry Dmitriev wrote:
> Hi Gerard,
>
> Overall looks good, but I have several comments & questions:
> 1) BiasedLockingBulkRevokeThreshold and BiasedLockingBulkRebiasThreshold
> Constraints for these flags states that "BiasedLockingBulkRevokeThreshold >= BiasedLockingBulkRebiasThreshold", thus I
> think that min range for BiasedLockingBulkRevokeThreshold should be >= than min range for
> BiasedLockingBulkRebiasThreshold. Currenty BiasedLockingBulkRevokeThreshold min range(0) is less than
> BiasedLockingBulkRebiasThreshold min range(1).
>

Good catch, fixed. The choices here were either to decrease BiasedLockingBulkRebiasThreshold min range to 0, or increase 
BiasedLockingBulkRevokeThreshold min range to 1. I went with the first choice (ie. 0) so that 
BiasedLockingBulkRebiasThreshold would fire up every time, if needed.


> 2) Typo in src/share/vm/runtime/commandLineFlagConstraintsRuntime.cpp on line 112: "BiasedLockingBulkRebiasThreshold"
> should be instead of "BiasedLockingBulkRevokeThreshold".

Fixed.


>
> 3) Error message in BiasedLockingDecayTimeFunc and BiasedLockingBulkRevokeThresholdFunc
> functions(commandLineFlagConstraintsRuntime.cpp module) states that ratio should be less or equal to 0.1, but "if"
> statement verifies that ">=0.1", i.e. 0.1 is not acceptable value.
>

Fixed.


> 4) I think that max range for "TraceRedefineClasses" should be "max_juint" instead of "0x80000000". Each bit in
> TraceRedefineClasses defines event to trace, here a part of the
> hotspot/src/share/vm/prims/jvmtiRedefineClassesTrace.hpp(lines 38-74):
> //    0x00000000 |          0 - default; no tracing messages
> //    0x00000001 |          1 - name each target class before loading, after
> //                              loading and after redefinition is completed
> ...
> //    0x00000010 |         16 - unused
> //    0x00000020 |         32 - unused
> //    0x00000040 |         64 - unused
> //    0x00000080 |        128 - unused
> //    0x00000100 |        256 - previous class weak reference addition
> //    0x00000200 |        512 - previous class weak reference mgmt during
> //                              class unloading checks (GC)
> ...
> //    0x40000000 | 1073741824 - unused
> //    0x80000000 | 2147483648 - unused
>
> Thus "0x80000000 == unused" mean that this bit just currently not used.
>

We know the range max can not be bigger than that (yes it's a bit value, but also bigger than all flags turned ON), so 
why not use the biggest >>known<< value here?


> 5) PrintSafepointStatisticsCount flag have range (1, max_intx), thus do we need following code in
> SafepointSynchronize::deferred_initialize_stat() function(hotspot/src/share/vm/runtime/safepoint.cpp)?
>
>    if (PrintSafepointStatisticsCount <= 0) {
>      fatal("Wrong PrintSafepointStatisticsCount");
>    }
>

Ah, very nice, more code to delete. Fixed.


> 6) PrintSafepointStatisticsTimeout max range currently is max_intx, but I think it should be divided by MICROUNITS,
> because PrintSafepointStatisticsTimeout is multiplied on MICROUNITS(lines 1171 and 1237
> hotspot/src/share/vm/runtime/safepoint.cpp) and this can lead to the overflow if PrintSafepointStatisticsTimeout is
> greater than (max_intx / MICROUNITS).
>

Nice! Fixed.


> 7) MaxJavaStackTraceDepth have max range equal to max_intx. This flag is assigned to int variables and sometimes
> multiplied by 2 and then assigned to the int variables. Thus, I think max range for this flags should be (max_jint / 2)
> to avoid overflow.
>

Another great catch! Fixed.


> 8) GuaranteedSafepointInterval have max range equal to 0x7FFFFFFF(lines 3117, 3122), but I believe that max_jint have
> the same value and can be used instead of 0x7FFFFFFF.
>

I believe I tried using max_intx before (not max_jint since the value is of intx type), but there was a problem on 
embedded platform. I will recheck to see what the exact problem was...


> 9) SafepointTimeoutDelay max range currently is max_intx, but I think it should be divided by MICROUNITS, because
> SafepointTimeoutDelay is multiplied on MICROUNITS(line 204 hotspot/src/share/vm/runtime/safepoint.cpp) and this can lead
> to the overflow if SafepointTimeoutDelay is greater than (max_intx / MICROUNITS).


Fixed.


>
> 10) VMThreadStackSize have max range equal to "max_intx". VMThreadStackSize is a size in Kbytes and code which use this
> flag multiply this flag on a "K" value, thus I think that max range should be "max_intx / (1 * K)".

Fixed.


>
> 11) PerfDataSamplingInterval have max range equal to "max_intx". It passed as constructor argument StatSamplerTask
> object, but StatSamplerTask object accepts "int" parameter, thus I think that max range for this flag should be "max_jint".

Fixed.


Very, very big thanks for such a thorough review Dmitry! Let me build, re-test and I will be back for a webrev1



cheers



>
> Thanks,
> Dmitry
>
> On 18.09.2015 19:49, 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.
>>
>
>

More information about the hotspot-runtime-dev mailing list