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

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Wed Oct 14 06:34:07 UTC 2015 

Hi Gerald, 

sorry I only look at this now.  
The PPC implementation is missing, the build fails right away.
I will send you a patch to apply on top of your change off-list
today.

Best regards,
  Goetz.




-----Original Message-----
From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-bounces at openjdk.java.net] On Behalf Of gerard ziemski
Sent: Dienstag, 13. Oktober 2015 19:19
To: Dmitry Dmitriev; hotspot-runtime-dev at openjdk.java.net; Coleen Phillimore
Subject: Re: RFR rev1 (M): 8078556: Runtime: implement ranges (optionally constraints) for those flags that have them missing

hi Dmitry,

Excellent findings. I lost a few changes due to the sync-up to TOT to get the GC flag fixes.


On 10/13/2015 11:03 AM, Dmitry Dmitriev wrote:
> Hi Gerard,
>
> Thank you for fixing issues from revision 0, overall looks good, but I found that some of these issues are still exist.
> Here are few small issues:
> 1) BiasedLockingBulkRevokeThresholdFunc constraint function still have(line 97, commandLineFlagConstraintsRuntime.cpp)
> "(double)value/(double)BiasedLockingDecayTime >= 0.1" instead of > 0.1 as in corrected BiasedLockingDecayTimeFunc
> constraint function.

Redone.

>
> 2) Following code still exist in hotspot/src/share/vm/runtime/safepoint.cpp module(lines 1055-1057):
>    if (PrintSafepointStatisticsCount <= 0) {
>      fatal("Wrong PrintSafepointStatisticsCount");
>    }
>
> PrintSafepointStatisticsCount flag have range (1, max_intx), thus as I remember we agreed that this code is redundant.
> Can you please remove them?
>

Redone.


> 3) You correct MaxJavaStackTraceDepth max range to max_intx/2 according to my comment from previous review, but not
> change max_intx to max_jint.  This flag is assigned to int variables and using max_intx can lead to overflow. Thus, I
> think that max_jint should be instead of max_intx.

This I think I missed, but you are right, on 64bit intx is 8 bytes, but int is 4, so we need max_jint (max_int) here.


> 4) I don't see changes to the TestOptionsWithRanges.java in rev2. It is possible that these changes are lost in sync
> with GC ranges or these changes are not more needed?

Redone.


I will need to re-submit RBT and JPRT to make 100% sure all is right here.


Thank you for the review!

> I don't need to see another webrev for that.
>
> Thank you,
> Dmitry
>
> On 13.10.2015 17:26, gerard ziemski wrote:
>> Thank you Yumin, Jiangli for the feedback.
>>
>> For the time being I'm going to leave AttachListenerTimeout range as is. It was defined by Staffan Larsen and we can
>> always tweak the value later if we have a good reason. I agree that the value looks awkward. The value is exactly 35
>> min, and why that is would be a question for Staffan.
>>
>> I have updated the webrev http://cr.openjdk.java.net/~gziemski/8078556_rev2 that only has 3 differences from rev1:
>>
>> #1 SharedSymbolTableBucketSize range min is now 2 [Jiangli]
>>
>> #2 Updated INITIAL_CONSTRAINTS_SIZE 45 [Dmitry, merged with GC flags changes]
>>
>> #3 Updated INITIAL_RANGES_SIZES 204 [Dmitry, merged with GC flags changes]
>>
>>
>> Testing with the new value passes JPRT and RBT tests.
>>
>> Dmitry, Coleen, can I please have your final reviews?
>>
>>
>> cheers
>>
>> On 10/12/2015 10:41 PM, Yumin Qi wrote:
>>> Hi, Gerard
>>>
>>>    I am not the definer for ShareTableBucketSize --- Jangli first checked int code for its definition:
>>>
>>> changeset:   7561:05c08ab3cf65
>>> parent:      7559:47ffd05828f9
>>> user:        jiangli
>>> date:        Wed Dec 17 23:34:52 2014 -0500
>>> summary:     8059510: Compact symbol table layout inside shared archive.
>>>
>>> changeset:   7561:05c08ab3cf65
>>> parent:      7559:47ffd05828f9
>>> user:        jiangli
>>> date:        Wed Dec 17 23:34:52 2014 -0500
>>> summary:     8059510: Compact symbol table layout inside shared archive.
>>>
>>>
>>> BTW, I only have one minor comment:
>>>
>>> *src/os/aix/vm/globals_aix.hpp: **product_pd(intx, AttachListenerTimeout, \*
>>> *"Timeout in ms the attach listener waits for a request") \*
>>> *+ range(0, 2147483) \ This is about 35 minutes. But the number looks awkward. I am ok if you keep it, better to change
>>> to a reasonable reading symbol. Thanks Yumin *
>>>
>>>
>>>
>>> On 10/9/15, 11:47 AM, gerard ziemski wrote:
>>>> (resending, fixed webrev link)
>>>>
>>>> hi all,
>>>>
>>>> Please review this rev1 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.
>>>>
>>>> Summary of changes since rev0:
>>>>
>>>> #1 Punted the following flags:
>>>>
>>>> SharedBaseAddress
>>>> SharedMiscCodeSize
>>>> SharedMiscDataSize
>>>> SharedReadOnlySize
>>>> SharedReadWriteSize
>>>>
>>>> to task JDK-8138983 since their ranges are nontrivial [Coleen, Jianghli]
>>>>
>>>> #2. Removed unused "Arguments::get_default_heap_base_min_address" [Coleen]
>>>>
>>>> #3. Users may need to use "TraceRedefineClasses" with their own custom events, so changed the max value to 0XFFFFFFFF
>>>> [Dmitry, Dan]
>>>>
>>>> #4. All other feedback from Dmitry implemented, see below
>>>>
>>>> #5. PrintSafepointStatisticsTimeout and SafepointtimeoutDelat max value is LP64_ONLY(max_intx/MICROUNITS)
>>>> NOT_LP64(max_intx) because intx is 4bytes on 32bits architectures, so dividing it by MICROUNITS would bring the value
>>>> below default. I did, however, make sure to upcast the values to jlong, so on 32bit they become 8byte long long, and
>>>> so no overflow will occur when multiplying by MICROUNITS.
>>>>
>>>> #6. GuaranteedSafepointInterval range(0, max_intx) asserts (overflows) on Windows64 (max_intx on 64bit is
>>>> 0x7FFFFFFFFFFFFFFF), so I changed the range max to max_jint (ie. 0x7FFFFFFF) which is roughly 25 days
>>>>
>>>>
>>>> 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
>>>> 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_rev1
>>>>
>>>> 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"
>>>> https://bugs.openjdk.java.net/browse/JDK-8138983 "Runtime: implement ranges for Shared*** flags"
>>>>
>>>> Thank you.
>>>>
>>>>
>
>

More information about the hotspot-runtime-dev mailing list