RFR 8130459(M): Add additional validation after heap creation

Thu Jul 23 05:50:04 UTC 2015

Hi Kim,

Thank you for the review!

On 07/22/2015 04:16 PM, Kim Barrett wrote:
> On Jul 20, 2015, at 3:48 PM, sangheon.kim <sangheon.kim at oracle.com> wrote:
>> Here's webrev.03 including your suggestion of 'removing change_to_enum()'.
>> http://cr.openjdk.java.net/~sangheki/8130459/webrev.03/
> ------------------------------------------------------------------------------
>
> I think I'm ok with the new functionality, though I'm having some
> trouble following parts of it.  I think that is in part because some
> things are not placed where I would expect them to be.  Here's a
> suggestion that I think might help:
>
> check_ranges should be a public static member function of
> CommandLineFlagRangeList, and its definition should be with that
> class.  CommandLineFlags::check_ranges should forward to it, or remove
> that forwarding function and call
> CommandLineFlagRangeList::check_ranges directly.
As I thought CommandLineFlags is a main class for command line flags, I 
wanted to concentrate all functionality there.
However separating check functionality to CommandLineFlagRangeList and 
CommandLineFlagConstraintList also seems reasonable.
So as you suggested I will move into CommandLineFlagRangeList and 
CommandLineFlagConstraintList.
And then we can eliminate some functions from CommandLineFlags class.

>
> check_constraints should be a public static member function of
> CommandLineFlagConstraintList, and its definition should be with that
> class.  It should set _validating_type to the argument (after
> verifying the argument is greater than current, or perhaps even the
> next value after current), and set_validating_type should be
> eliminated.
Okay.

>
> CommandLineFlags::check_constraints_of_after_xxx functions should call
> CommandLineFlagConstraintList::check_constraints.  Alternatively,
> eliminate those functions and instead call
> CommandLineFlagConstraintList::check_constraints directly or via a
> CommandLineFlags::check_constraints forwarding function.  [Be
> consistent with check_ranges about forwarding function or not.]
Okay, I will directly call 
CommandLineFlagConstraintList::check_constraint() and 
CommandLineFlagRangeList::check_range().

>
> Eliminating the check_constraints_of_after_xxx functions would also
> involve moving the call to post_after_ergo_constraint_check, but I
> think that's misplaced inside
> CommandLineFlags::check_constraints_of_after_ergo.
I will change to call post_after_ergo_constraint_check directly since 
CommandLineFlags will not have constraint/range related functions.
But I don't think it is misplaced as CommandLineFlags managed all 
constraint/range check functions, adding post work function there seemed 
natural.
If we need 'AfterMemoryInit' post work, I was planing to add in 
CommandLineFlags::check_constraints_of_after_memory_init(). :)

>
> Eliminating those check_constraints_of_after_xxx functions would have
> the additional benefit of removing any concerns about their names; I
> think another reviewer (David?) commented on them, and they strike me
> as rather strange.
I will change them.
But again I wanted to centralize range/constraint related functions at 
CommandLineFlags class. :-)

>
> ------------------------------------------------------------------------------
>
> Why do we need check_ranges?  Aren't ranges always checked when a
> value is assigned?  I'm probably forgetting something, but I can't
> right now find or recall a reason for needing this late check.  Oh,
> maybe it's to validate the initial values for options that didn't get
> set via command line or ergonomics?  If so, perhaps check_ranges
> should be run much earlier?  But maybe some options don't have a value
> at all until after apply_ergo is called?
>
> This should be a separate discussion / RFE.
>
> ------------------------------------------------------------------------------
> The name post_after_ergo_constraint_check seems really odd.  I
> understand how it comes about, but still...
>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintList.cpp
>   223 #define INITIAL_CONTRAINTS_SIZE 16
> ...
>   230   _constraints = new (ResourceObj::C_HEAP, mtInternal) GrowableArray<CommandLineFlagConstraint*>(INITIAL_CONTRAINTS_SIZE, true);
>
> Pre-existing typo: INITIAL_CONTRAINTS_SIZE => INITIAL_CONSTRAINTS_SIZE
Okay.

>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintList.hpp
>    71   const char* name() { return _name; }
>    72   ConstraintType type() { return _validate_type; }
>
> type() should be const.  Pre-existing: name() should be const.
Okay.

>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>    50                   "ergornomic PLAB minimum size (" SIZE_FORMAT ")\n",
> ...
>    65                   "ergornomic PLAB maximum size (" SIZE_FORMAT ")\n",
>
> "ergornomic" => "ergonomic"
Okay.

>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.cpp
> 1248     CommandLineFlagRange* range = CommandLineFlagRangeList::at(i);
> 1249     const char* name = range->name();
> 1250     Flag* flag = Flag::find_flag(name, strlen(name), true, true);
> 1251     if (flag != NULL) {
>
> [Pre-existing code, though moved as part of this change.]
>
> I don't understand the need for the "if (flat != NULL) {" wrapper.
> Range objects are only created and registered with flags, so it
> shouldn't be possible for that test to ever fail.  Note that the
> constraint processing doesn't have such a test.
>
> [Doing anything about this should be a separate RFE, and this question
> is really directed more toward Gerard rather than Sangheon.]
Gerard already answered.

>
> ------------------------------------------------------------------------------
> src/share/vm/runtime/globals.cpp
> 1213 bool CommandLineFlags::check_ranges() {
> 1214 //#define PRINT_RANGES_SIZES
> 1215 #ifdef PRINT_RANGES_SIZES
> ...
> 1274 static bool check_constraints(CommandLineFlagConstraint::ConstraintType constraint_type) {
> 1275 //#define PRINT_CONSTRAINTS_SIZES
> 1276 #ifdef PRINT_CONSTRAINTS_SIZES
>
> [More or less pre-existing code, though moved and updated as part of
> this change.]
>
> Do we really need this development cruft cluttering the code?
>
> [Doing anything about this should be a separate RFE, and this question
> is really directed more toward Gerard rather than Sangheon.]
Gerard already answered.

>
> ------------------------------------------------------------------------------
>

I will upload webrev.04 (w/ incremental webrev) after running JPRT.

Thanks,
Sangheon