RFR 8078555(M): GC: implement ranges (optionally constraints) for those flags that have them missing
sangheon.kim
sangheon.kim at oracle.com
Thu Aug 20 16:42:28 UTC 2015
Hi Gerard,
You are right.
Now I fixed both webrev.01 and webrev.01_to_00.
Thank you for finding them!
Sangheon
On 08/20/2015 09:17 AM, gerard ziemski wrote:
> hi Sangheon,
>
> Is it me or is the patch.01 out of sync with the actual webrev.01?
>
> I tried applying the patch from your webrev.01, but the resulting code
> produced does not match that of the webrev itself - the
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp in particular
> does not match.
>
> Would you kindly double check that and fix it if it's not just me?
>
>
> cheers
>
> On 08/20/2015 01:31 AM, sangheon.kim wrote:
>> Hi Kim and Dmitry,
>>
>> Here's new webrev.
>>
>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.01/
>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.01_to_00/
>>
>> And let me correct one of my answers to Kim.
>>>> I see that the Check helper is being used both here and for
>>>> MaxHeapSize constraint function. I think the helper needs to be
>>>> called with **both** values, and not refer to the current values at
>>>> all. The SoftRefXXX checker would call Check with the argument value
>>>> and the current value of MaxHeapSize. The MaxHeapSize checker would
>>>> call Check with the current value of SoftRefXXX and the argument
>>>> value.
>>>> Oh, right.
>>>> I will fix this.
>>> Oh, right.
>>> I will fix this.
>> As you already mentioned, validation check can be done for only one flag
>> at a time.
>> So helper function can't called with BOTH new values.
>> But I think there's no problem with this as eventually these new values
>> will be compared.
>> e.g. flagA = 10(default 5), flagB = 20(default 5) and constraint: flagA
>> * flagB <= 150
>> constraint functions will be called for their declared order.
>> when constraint function for flagA is called: 10(new flagA) * 5(default
>> flagB) -> PASS
>> when constraint function for flagB is called: 10(new flagA) * 20(new
>> flagB) -> FAIL.
>>
>> Thanks,
>> Sangheon
>>
>>
>> On 08/19/2015 03:58 PM, sangheon.kim wrote:
>>> Hi Kim,
>>>
>>> Thank you for reviewing this.
>>>
>>> On 08/19/2015 02:59 PM, Kim Barrett wrote:
>>>> On Aug 19, 2015, at 12:25 AM, sangheon.kim <sangheon.kim at oracle.com>
>>>> wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this patch for command-line validation for GC flags.
>>>>> This includes adding ranges and implementing constraint functions
>>>>> for GC flags.
>>>>>
>>>>> […]
>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8078555
>>>>> Webrev: http://cr.openjdk.java.net/~sangheki/8078555/webrev.00/
>>>>> Testing: JPRT, UTE(vm.quick-pcl), tests at test/runtime/CommandLine
>>>>> and local tests[1].
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/share/vm/gc/g1/g1_globals.hpp
>>>> 87 product(size_t, G1SATBBufferSize,
>>>> 1*K, \
>>>> 88 "Number of entries in an SATB log
>>>> buffer.") \
>>>> 89 range(1,
>>>> max_uintx) \
>>>>
>>>> Shouldn't that be max_size_t? Oh, wait; we don't have that. Maybe we
>>>> should.
>>> It would be nice to have.
>>> Let me handle from follow up CR.
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/share/vm/runtime/arguments.cpp
>>>> 583 static bool set_fp_numeric_flag(char* name, char* value,
>>>> Flag::Flags origin) {
>>>> 584 errno = 0;
>>>> 585 double v = strtod(value, NULL);
>>>> 586 if (errno == ERANGE) {
>>>> 587 return false;
>>>> 588 }
>>>>
>>>> You and Dmitry already discussed checking the endptr. Good catch,
>>>> Dmitry.
>>>>
>>>> I suggest returning false for any non-zero errno value.
>>> Okay, I will change to (errno != 0).
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/share/vm/runtime/arguments.cpp
>>>> 722 #define NUMBER_RANGE "[0123456789eE+]"
>>>>
>>>> Missing "-" for negative exponents.
>>> This definition is used for below decimal point. So we don't need '-'
>>> here.
>>> Currently we can reach strtod function above with '-1.1'.
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>>> 48 static Flag::Error
>>>> ParallelGCThreadsAndCMSWorkQueueDrainThreshold(bool verbose) {
>>>> 61 Flag::Error ParallelGCThreadsConstraintFunc(uint value, bool
>>>> verbose) {
>>>> 90 Flag::Error ConcGCThreadsConstraintFunc(uint value, bool
>>>> verbose) {
>>>>
>>>> These are associated with collectors that are excluded when
>>>> INCLUDE_ALL_GCS is false. The bodies of these should be conditionally
>>>> included based on that macro, and they should just return
>>>> Flag::SUCCESS when the macro is false.
>>> Okay.
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>>> 142 Flag::Error OldPLABSizeConstraintFunc(size_t value, bool
>>>> verbose) {
>>>> 143 if (UseConcMarkSweepGC) {
>>>>
>>>> The UseConcMarkSweepGC clause should be conditionalized out if
>>>> INCLUDE_ALL_GCS is false.
>>>>
>>>> Peeking ahead, it looks like there are quite a few constraint
>>>> functions that should be similarly conditionalized.
>>> Okay.
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>>> 183 static Flag::Error
>>>> CheckMaxHeapSizeAndSoftRefLRUPolicyMSPerMB(bool verbose) {
>>>> ...
>>>> 196 Flag::Error SoftRefLRUPolicyMSPerMBConstraintFunc(intx value,
>>>> bool verbose) {
>>>> 197 return CheckMaxHeapSizeAndSoftRefLRUPolicyMSPerMB(verbose);
>>>> 198 }
>>>>
>>>> This doesn't look right. The Check helper function is using the
>>>> current value of SoftRefLRUPolicyMSPerMB, and doesn't have access to
>>>> the prospective new value, since the constraint function isn't passing
>>>> it.
>>>>
>>>> I see that the Check helper is being used both here and for
>>>> MaxHeapSize constraint function. I think the helper needs to be
>>>> called with *both* values, and not refer to the current values at
>>>> all. The SoftRefXXX checker would call Check with the argument value
>>>> and the current value of MaxHeapSize. The MaxHeapSize checker would
>>>> call Check with the current value of SoftRefXXX and the argument
>>>> value.
>>> Oh, right.
>>> I will fix this.
>>>
>>>>
>>>> Otherwise, updates that validate the value before performing the
>>>> assignment will be using the old value for validation, rather than
>>>> checking the new value as intended.
>>>>
>>>> This also brings up an interesting limitation of the present
>>>> mechanism. If two options are related in some way, both need to have
>>>> constraint checking functions to ensure the relationship remains valid
>>>> after initial option checking. But this means that the initial option
>>>> checking will check the relationship twice, and if there is a problem
>>>> it will be reported twice. I don't have any easy suggestions for how
>>>> to improve on this situation though.
>>> Right.
>>> This is current limitation.
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> src/share/vm/runtime/globals.hpp
>>>> 1854 range(0,
>>>> MarkStackSizeMax) \
>>>>
>>>> Does this actually work? I'm not sure when the range values are
>>>> evaluated. I suspect this wouldn't work properly in the face of later
>>>> updates to MarkStackSizeMax.
>>> Correct.
>>> I will change their order to be 'MarkStackSizeMax' first.
>>>
>>> I will send next version of webrev soon.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>>
>>>
>>
>>
>>
More information about the hotspot-dev
mailing list