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

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 DBLMAX. 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.00to8078555 Testing: JPRT, UTE(vm.quick-pcl) and test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java. ------------------------------------------------------------------------------ src/share/vm/gc/g1/g1globals.hpp 142 product(intx, G1ConcRefinementServiceIntervalMillis, _300, _ ... 145 range(0, _maxjint) _ Why is this being changed from maxintx to maxjint? This option is used for 'long' type parameter and it is 32bit for Windows. ------------------------------------------------------------------------------ src/share/vm/gc/g1/g1globals.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/g1globals.hpp 240 product(uintx, G1ConcRefinementThreads, _0, _ ... 243 range(0, _(maxjint-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