RFR 8134995(M): [REDO] GC: implement ranges (optionally constraints) for those flags that have them missing
Jon Masamitsu
jon.masamitsu at oracle.com
Tue Sep 22 23:04:51 UTC 2015
On 9/21/2015 4:05 PM, sangheon.kim wrote:
> 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'.
Sangheon,
I think your suggestion (when we talked today) about dividing by jintSize
should work.
Kim,
There are more logical maximum values that could be reverse-engineered
from the
implementation but I thought to keep it simple.
>
>>
>> ------------------------------------------------------------------------------
>>
>> 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.
Kim,
My apologies but I dissuaded Sangheon from that idea. I don't think the
number of processors was enough. A hundred or a thousand times the
number of processors should be enough but if I had to pick some
multiplier like that, I'd rather pick something limited by the size
of the type and back off some. It's still arbitrary but whereas I might
some decade be surprised that 1000 * #-of-processors was too few,
I'm not going to see 1,000,000,000 be too few.
In both these situations if I had to explain myself, I'd rather says
something about arithmetic overflow than that I just thought
it was big enough.
Jon
> 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