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

sangheon.kim sangheon.kim at oracle.com
Mon Sep 21 23:05:31 UTC 2015


Hi Kim,

Thanks for looking at this.

On 09/21/2015 02:59 PM, Kim Barrett wrote:
> On Sep 10, 2015, at 8:01 PM, sangheon.kim <sangheon.kim at oracle.com> 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.
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1_globals.hpp
>   142   product(intx, G1ConcRefinementServiceIntervalMillis, 300,                 \
> ...
>   145           range(0, max_jint)                                                \
>
> Why is this being changed from max_intx to max_jint?
This option is used for 'long' type parameter and it is 32bit for Windows.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1_globals.hpp
>   169   develop(intx, G1RSetRegionEntriesBase, 256,                               \
>   173   product(intx, G1RSetRegionEntries, 0,                                     \
>   179   develop(intx, G1RSetSparseRegionEntriesBase, 4,                           \
>   184   product(intx, G1RSetSparseRegionEntries, 0,                               \
>   240   product(uintx, G1ConcRefinementThreads, 0,                                \
>
> All of these have their max range value changed to be divided by 4.
> What's this about?  What is this magic "4"?
'G1RSetRegionEntries', 'G1RSetSparseRegionEntries' and 
'G1ConcRefinementThreads' are finally multiplied by pointer size.
So we need to divide by 4 for 32bit to avoid overflow. We don't need for 
64bit but I thought it will be enough for 64bit.
'G1RSetRegionEntriesBase' and 'G1RSetSparseRegionEntriesBase' changed to 
have same range as 'G1RSetRegionEntries' and 'G1RSetSparseRegionEntries'.

>
> ------------------------------------------------------------------------------
> src/share/vm/gc/g1/g1_globals.hpp
>   240   product(uintx, G1ConcRefinementThreads, 0,                                \
> ...
>   243           range(0, (max_jint-1)/4)                                          \
>
> I know we discussed the problem of thread counts.  I'd suggested
> perhaps basing the upper bound on the number of processors.  (Some
> care might be needed for uniprocessor systems.) I couldn't find any
> followup on that suggestion though.
Firstly I agreed to your opinion of using the number of processors if we 
need to give upper bound.
But from the email thread that we discussed, I ended not to handle OOM 
from our range/constraint validation framework.
Dmitry told us that testing framework is okay for OOM(only for exit 
value of 1) and Gerard also agreed not to handle it from validation.
Finally, I think the only thing that we need for this option is just 
excluding this test.

If we don't exclude tests that are using too many resources, it would 
make problem to other tests that running in parallel.
So it would be better to exclude these tests.

Thanks,
Sangheon


> Such an approach would probably
> let us remove this change:
>
> test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>    58         allOptionsAsMap.remove("G1ConcRefinementThreads");
>
> Note also that this
> test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>    52         allOptionsAsMap.remove("CICompilerCount");
> is another example of an impossibly huge thread count.
>   
> ------------------------------------------------------------------------------
>



More information about the hotspot-dev mailing list