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

Thu Aug 20 08:18:02 UTC 2015

Hi Kim,

>> 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'. 
Sorry, I misunderstood your point!
You are right to parse negative exponential we need '-' too.
e.g. '1.1e_*-*_308'.

This is also reflected to webrev.01.

Thanks,
Sangheon

On 08/19/2015 11:31 PM, 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
>>
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>
>