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

Thu Aug 20 06:31:17 UTC 2015

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
>
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>