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

[Dmitry Dmitriev]

Hello Sangheon,

I have one comment about changed set_fp_numeric_flag function in 
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   }

I think that second parameter for strtod also should be checked to 
ensure that valid exponential notation is passed. For example, currently 
it is possible to pass following notation: "1.5ee10" or "1.5e++10" and 
value will be set to 1.5

Also, I still have opinion that DBL_MAX is a not great upper range for 
double options, but I agree that we can leave it not and probably find 
more appropriate value in the future enhancement. For example, 
CMSSmallCoalSurplusPercent(and similar) have upper range equal to 
DBL_MAX. I looked at this flag and it used in multiplication and then 
result is converted to ssize_t(line 2129 in 
src/share/vm/gc/cms/compactibleFreeListSpace.cpp module) and these can 
lead to some overflow problems. So, it one of the reason why I think 
that we need to find more precise upper range. This can be considered as 
future enhancement.

Thank you,
Dmitry

On 19.08.2015 7:25, sangheon.kim 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.
>
> Here are several things to note.
> 1. Exponential notation for 'double' type variable parse: Previously 
> there were some discussion for maximum value for double type flags 
> from code review of JDK-8059557 and JDK-8112746. And Kim and I decided 
> not to add upper limit unless there are problems with DBL_MAX. And as 
> 255 is the maximum length that can be passed via command-line, we 
> introduced exponential notation to avoid this limit. ( arguments.cpp )
> 2. These GC flags ranges are not ideal ranges but ranges which don't 
> make problem with current source code.
>     If one flag makes some problem but hard to find good range, I 
> added some ranges.
> 3. There are some constraint functions to avoid overflow.
> 4. Test applications are changed: as some of them assumed to be 
> ParallelGC or to check it's output messages.
> 5. Includes cleanup of JDK-8133565: GC -2nd followup to JDK-8059557.
>
> 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].
>
> [1]: Running GCOld for non bool GC flags with its variable type's 
> min/max/median. Needed real GCs to verify whether flags are making 
> problems or not.
> (e.g. java -XX:YoungPLABSize={min_uintx, max_uintx, max_uintx/2} GCOld 
> 10 200 100 20 5000
>
> Thanks,
> Sangheon