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

[Dmitry Dmitriev]

Sangheon,

Few comments inline.

On 19.08.2015 19:25, sangheon.kim wrote:
> 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?
Yes, I think that these values should not be parsed and JVM should 
report error. For example if I put two "ee", then I should get an error. 
Currently "1.5ee10" value is parsed and double flag is equal to "1.5" 
isntead of "1.5e10".

> Let me change this at next version. :)
Thank you!
>
>>
>> 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.
Yes, sure!

Thanks,
Dmitry
>
> 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
>>
>