RFR 8134995(M): [REDO] GC: implement ranges (optionally constraints) for those flags that have them missing

[sangheon.kim]

On 10/01/2015 03:06 PM, Jon Masamitsu wrote:
>
>
> On 10/01/2015 12:10 PM, sangheon.kim wrote:
>> Hi Jon,
>>
>> On 10/01/2015 11:57 AM, Jon Masamitsu wrote:
>>> Sangheon,
>>>
>>> What was the failure that prompted this change?
>> test/gc/g1/mixedgc/TestLogging.java
>> And as I answered to Kim, this test is wrote correctly.
>>
>> Current constraint function is trying to verify below:
>> MaxGCPauseMillis >= GCPauseIntervalMillis
>>
>> But if one of them is using default value, this comparison is not 
>> valid as default value means set it ergonomically.
>> This is why I added FLAG_IS_CMDLINE().
>
> OK.  I understand better now.  Perhaps you could add a comment
> such as this to the top of the file.
>
> // Some flags that have default values that indicate that the
> // JVM should automatically determine an appropriate value
> // for that flag.  In those cases it is only appropriate for the
> // constraint checking to be done if the user has specified the
> // value(s) of the flag(s) on the command line.  In the constraint
> // checking functions,  FLAG_IS_CMDLINE() is used to check if
> // the flag has been set by the user and so should be checked.
Okay, I will add this explanation at the top of the file.

Sangheon


>
> Jon
>
>>
>> Sangheon
>>
>>
>>> I
>>> don't understand why the check for FLAG_IS_CMDLINE()
>>> is needed.
>>>
>>> Jon
>>>
>>> On 10/01/2015 09:24 AM, sangheon.kim wrote:
>>>> Hi all,
>>>>
>>>> During this code review, newly added gc test found a problem of 1 
>>>> gc constraint function(MaxGCPauseMillisConstraintFunc).
>>>>
>>>> So I updated it and related function 
>>>> (GCPauseIntervalMillisConstraintFunc).
>>>> - Added checking for MaxGCPauseMillis and GCPauseIntervalMillis 
>>>> whether these are set via commandline or not to proceed the 
>>>> constraint function logic.
>>>>
>>>> http://cr.openjdk.java.net/~sangheki/8134995/webrev.03
>>>> http://cr.openjdk.java.net/~sangheki/8134995/webrev.03_to_02/
>>>>
>>>> * Testing: JPRT and RBT(to manually test runtime/CommandLine for 
>>>> embedded)
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>> On 09/25/2015 03:52 PM, sangheon.kim wrote:
>>>>> Hi Gerard,
>>>>>
>>>>> I found that I missed your comment from webrev.01.
>>>>> Here's a new one which includes yours for TestOptionsWithRanges.java.
>>>>>
>>>>> http://cr.openjdk.java.net/~sangheki/8134995/webrev.02/
>>>>> http://cr.openjdk.java.net/~sangheki/8134995/webrev.02_to_01
>>>>>
>>>>> Thanks,
>>>>> Sangheon
>>>>>
>>>>>
>>>>> On 09/14/2015 07:23 AM, gerard ziemski wrote:
>>>>>> Thank you. I have no more comments - reviewed.
>>>>>>
>>>>>>
>>>>>> cheers
>>>>>>
>>>>>>
>>>>>> On 09/12/2015 03:38 AM, sangheon.kim wrote:
>>>>>>> Hi Gerard,
>>>>>>>
>>>>>>> On 09/11/2015 12:24 PM, sangheon.kim wrote:
>>>>>>>> Hi Gerard,
>>>>>>>>
>>>>>>>> Thank you for looking at this.
>>>>>>>>
>>>>>>>> On 09/11/2015 11:13 AM, gerard ziemski wrote:
>>>>>>>>> hi Sangheon,
>>>>>>>>>
>>>>>>>>> #1 
>>>>>>>>> test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java 
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please change the comment to:
>>>>>>>>>
>>>>>>>>> +        /*
>>>>>>>>> +         * Exclude below options as their maximum value would 
>>>>>>>>> consume too much memory
>>>>>>>>> +         * and would affect other tests that run in parallel.
>>>>>>>>> +         */
>>>>>>>> Okay, I will fix as you suggested.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> #2 What tests did you run? Did you run 
>>>>>>>>> test/runtime/CommandLine/OptionsValidation on all platforms 
>>>>>>>>> (including embedded)?
>>>>>>>> No.
>>>>>>>> I ran tests under test/runtime/CommandLine/OptionsValidation 
>>>>>>>> (especially TestOptionsWithRanges.java) for all platforms
>>>>>>>> except embedded.
>>>>>>>> Let me back after testing on embedded.
>>>>>>> I ran for embedded (linux-arm64, linux-armvh, linux-armvfpsflt, 
>>>>>>> linux-armvfphflt, linux-armsflt) and all of them PASSED
>>>>>>> for test/runtime/CommandLine/OptionsValidation.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Sangheon
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Sangheon
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> cheers
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 09/10/2015 07:01 PM, sangheon.kim wrote:
>>>>>>>>>> Hi all,
>>>>>>>>>>
>>>>>>>>>> Please review this patch for command-line validation for GC 
>>>>>>>>>> flags.
>>>>>>>>>> This REDO patch is adding ranges and implementing constraint 
>>>>>>>>>> functions for GC flags.
>>>>>>>>>>
>>>>>>>>>> Original CR of JDK-8078555 was backout as it made a test 
>>>>>>>>>> failure from 'TestOptionsWithRanges.java'.
>>>>>>>>>> And also there were some discussion of OOM handling.
>>>>>>>>>>
>>>>>>>>>> Most parts are same as JDK-8078555 except below:
>>>>>>>>>> 1. Changed 'range' for some flags.
>>>>>>>>>> 2. Excluded 3 flags for TestOptionsWithRanges.java test. 
>>>>>>>>>> These flags make this test unstable as it tries to allocate
>>>>>>>>>> huge amount of memory.
>>>>>>>>>>
>>>>>>>>>> And below are the suggestion note for JDK-8078555:
>>>>>>>>>> 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-8134995
>>>>>>>>>>
>>>>>>>>>> Webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~sangheki/8134995/webrev.00/
>>>>>>>>>> http://cr.openjdk.java.net/~sangheki/8134995/webrev.00_to_8078555 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Testing:
>>>>>>>>>> JPRT, UTE(vm.quick-pcl) and 
>>>>>>>>>> test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java. 
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Sangheon
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>