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

[sangheon.kim]

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

>
> ------------------------------------------------------------------------------
>