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
Thu Oct 1 22:06:38 UTC 2015

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 FLAGISCMDLINE().

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.

Jon

Sangheon

I don't understand why the check for FLAGISCMDLINE() 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.03to02/ * 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.02to01 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 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. Thanks, Sangheon

More information about the hotspot-dev mailing list