RFR 8130459(M): Add additional validation after heap creation
Kim Barrett
kim.barrett at oracle.com
Wed Jul 22 23:16:23 UTC 2015
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.
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.
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.]
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.
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.
------------------------------------------------------------------------------
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
------------------------------------------------------------------------------
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.
------------------------------------------------------------------------------
src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
50 "ergornomic PLAB minimum size (" SIZE_FORMAT ")\n",
...
65 "ergornomic PLAB maximum size (" SIZE_FORMAT ")\n",
"ergornomic" => "ergonomic"
------------------------------------------------------------------------------
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.]
------------------------------------------------------------------------------
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.]
------------------------------------------------------------------------------
More information about the hotspot-dev
mailing list