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

[Kim Barrett]

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.

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

------------------------------------------------------------------------------ 
src/share/vm/runtime/arguments.cpp 
 722 #define        NUMBER_RANGE    "[0123456789eE+]"

Missing "-" for negative exponents.

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

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

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

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.

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

------------------------------------------------------------------------------