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

[sangheon.kim]

Hi Dmitry,

Thank you for looking at this!

On 08/19/2015 07:18 AM, Dmitry Dmitriev wrote:
> 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
Yes, right.
When I changed this routine I knew them but I thought it is okay.
You are pointing these values should not parsed, right?
Let me change this at next version. :)

>
> 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.
Yes, there would happen overflow.
CMSSmallCoalSurplusPercent and its friends have same behavior.
(ssize_t type) * (double type) is saved at (ssize_t type)
Fortunately, these flags with DBL_MAX doesn't hit assert or crash during 
running GCOld.
As you mentioned, we can leave them for future enhancement.

Thanks,
Sangheon


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