Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments

[Gerard Ziemski]

Thank you Derek very much for your feedback. Please see my answers inline:

> Subject:	Re: Revision2: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
> Date:	Mon, 01 Jun 2015 18:13:23 -0400
> From:	Derek White<derek.white at oracle.com>
> Organization:	Oracle
> To:	hotspot-dev at openjdk.java.net
>
> Hi Gerard,
>
> Still going over arguments.cpp and globals.hpp...
>
> Here are a few issues found so far:
>
> ----------------------------------------
> g1_globals.hpp:
> ----------------------------------------
>   - G1ConcMarkStepDurationMillis: The range is declared as range(1.0,
> (double)max_uintx). But max_uintx has nothing to do with max double. It
> may not even be precisely representable as a double.
>
The original code just specified min=1.0 and did not cap it, so I had to provide some valid value. I decided against using FLT_MAX, because I would need “#include <float.h>” just for that instance (also I am not 100% sure it’s available on all platforms). The max does not need to be mathematically ideal value, so long as it makes sense and "(double)max_uintx” is a good value, even if it might not be represented exactly.


>   - G1NewSizePercent: The constraint is checks that that its is <=
> G1MaxNewSizePercent, but not that it's >= 0. I assume that the uintx
> type is checking that?

That is correct.


> ----------------------------------------
> arguments.cpp:
> ----------------------------------------
> - Line 1405 - where did this come from?
>    if (SurvivorAlignmentInBytes == 0) {
>      SurvivorAlignmentInBytes = ObjectAlignmentInBytes;
>    }

It came from the old "verify_object_alignment()” which I re-implemented as constraints and moved the setter code in question to “set_object_alignment()”, which seems a much more ideal solution.


> ----------------------------------------
> globals.hpp:
> ----------------------------------------
>   - The variables StringTableSize and SymbolTableSize used to have ranges
>        - minimumStringTableSize..(max_uintx / StringTable::bucket_size()
>        - minimumSymbolTableSize..(max_uintx / SymbolTable::bucket_size()
>    Now they are:
> - range(minimumStringTableSize, 111*defaultStringTableSize)
>       - range(minimumSymbolTableSize, 111*defaultSymbolTableSize)
>    This is OK? I didn't have a chance to calculate these myself.

The max value must be some “sane” value that actually works - the main point of this exercise is to only allow values that are guaranteed not to crash the VM. The previous max values might be mathematically correct, but they will crash the VM, so we need smaller values.

Based on my previous experience with table sizes (ie. dynamically resizing the system hashtables) where I was privileged to be able to run several different performance benchmarks (with the help from the performance team) with different table sizes, I can say with confidence that size even an order of magnitude larger than the default is really big. I have allowed for 2 order’s of magnitude (ie. 111), which should be more than satisfactory and works.


>   - MinMetaspaceFreeRatio: used to have range 0..99, now it has range
> 0..100 and constraint <= MaxMetaspaceFreeRatio.
>      - Not sure how important.
>      - Actually the GC, runtime, and compiler groups should check all of
> their "percent" variables to see if having tighter ranges makes sense.

Oh yes, it should be 0..99

Fixed.


> Line 2036, 2041:
>    - Set status to false instead of returning directly?

I’m sorry, which file do those lines refer to?


> Line 2827 (existing); Change "K" -> "1*K" for consistency.

Line 2827 in gobals.hpp or is it a different file?


> ----------------------------------------
> commandLineFlagConstraintList.hpp
> ----------------------------------------
> - Initialize CommandLineFlagConstraintList._constraints in constructor
> to avoid dealing with NULL case?

Currently all the APIs are static. If I wanted to use the constructor, then I would need to create an instance of the object to use the APIs and some singleton API to retrieve the instance, which I am not sure that it would be worth it. The current NULL checks are pretty simple as is the class itself.


> - Shouldn't the default implementations for
> CommandLineFlagConstraint::apply_XXX() return failure, not success?

I looked at it from the point of view of the more likely scenario, which is assume success, unless a fault detected.

If we did do this, though, then instead of:

Flag::Error MinHeapFreeRatioConstraintFunc(bool verbose, uintx* value) {
   if ((CommandLineFlags::finishedInitializing()) && (*value > MaxHeapFreeRatio)) {
     if (verbose == true) {
       jio_fprintf(defaultStream::error_stream(),
                   "MinHeapFreeRatio (" UINTX_FORMAT ") must be less than or "
                   "equal to MaxHeapFreeRatio (" UINTX_FORMAT ")\n",
                   *value, MaxHeapFreeRatio);
     }
     return Flag::VIOLATES_CONSTRAINT;
   } else {
     return Flag::SUCCESS;
   }
}

we’d have:

Flag::Error MinHeapFreeRatioConstraintFunc(bool verbose, uintx* value) {
   Flag::Error error = Flag::VIOLATES_CONSTRAINT;
   if ((CommandLineFlags::finishedInitializing()) && (*value > MaxHeapFreeRatio)) {
     if (verbose == true) {
       jio_fprintf(defaultStream::error_stream(),
                   "MinHeapFreeRatio (" UINTX_FORMAT ") must be less than or "
                   "equal to MaxHeapFreeRatio (" UINTX_FORMAT ")\n",
                   *value, MaxHeapFreeRatio);
     }
   } else {
     error = Flag::SUCCESS;
   }
   return error;
}

which is not as nice, in my opinion.


> ----------------------------------------
> commandLineFlagConstraintsGC.cpp
> ----------------------------------------
> Is this necessary?:
>    37 #include "c1/c1_globals.hpp"
>    40 #include "opto/c2_globals.hpp”

Fixed.

> BAD TYPO:
>   - CMSOldPLABMaxConstraintFunc() is comparing against itself, not
> CMSOldPLABMin.
>
Fixed.

Thank you and I will be posting a new webrev shortly.


cheers