RFR 8130459(M): Add additional validation after heap creation
sangheon.kim at oracle.com
Thu Jul 16 01:46:30 UTC 2015
Thank you for reviewing this.
On 07/15/2015 10:34 AM, gerard ziemski wrote:
> hi Sangheon,
> I like how you map String constraint types into enums, so we don't
> have to do string comparisons later.
> I only have two comments:
> #1 src/share/vm/runtime/commandLineFlagConstraintList.hpp
> line 93: I don't like the "find_if_validated()" name for the API. If I
> understand the intend of the API correctly it should be something like
> "find_if_needs_check()", or better yet
Yes, I agree.
Changed to "find_if_needs_check().
> #2 src/share/vm/runtime/commandLineFlagConstraintList.hpp
> line 96, 98: We seem to be mixing up the API name convention used
> here. I used something_foo(), but you are using somethingFoo(), can
> you please change validateAfterParse() and setValidatingType() to
> validate_after_parse() and set_validating_type() respectively?
I missed that.
Here's updated webrev which contains:
- Gerard's comments for function names.
- Eric's comment for error message.
- Changed CommandLineFlagConstraintList::_validationType to
> On 07/15/2015 02:33 AM, sangheon.kim wrote:
>> Hi all,
>> Please review this change of adding additional validation on
>> command-line flag checking framework.
>> Recently we introduced Command-line flag checking framework
>> (JDK-8059557 <https://bugs.openjdk.java.net/browse/JDK-8059557>, JEP
>> 245: Validate JVM Command-Line Flag Arguments ) for ranges and
>> And most cases this works great.
>> Unfortunately there are some flags which can be checked after heap
>> creation and these flags are hard to be checked under current framework.
>> In this regard, I suggest to have an additional constraint checking.
>> This suggestion adds one more constraint checking stage after memory
>> And if we need more validations we can extend it.
>> There are 2 big changes.
>> 1. Flag declaration.
>> 'constraint(FUNC)' -> 'constraint(FUNC,TYPE)'
>> This webrev has 3 types, Anytime, AfterParse and AfterMemoryInit.
>> - Anytime: current constraint functions which don't use
>> - AfterParse: constraint functions which use
>> - AfterMemoryInit: I added only YoungPLABSize as an example.
>> * All GC related flags will be covered by JDK-8078555
>> <https://bugs.openjdk.java.net/browse/JDK-8078555>: GC: implement
>> ranges (optionally constraints) for those flags that have them missing.
>> 2. Previous constraint functions have to use
>> 'CommandLineFlags::finishedInitializing()' if the flag needs to be
>> checked after Argument::parse().
>> However, with this suggestion there's no need to call the function.
>> Instead framework will decide when the constraint function is needed
>> to be called.
>> And previously constraint functions are called twice while this
>> change will call each constraint function only one time.
>> CR: https://bugs.openjdk.java.net/browse/JDK-8130459
>> webrev: http://cr.openjdk.java.net/~sangheki/8130459/webrev.00/
>> Testing: JPRT, UTE(vm.quick-pcl) and tests at test/runtime/CommandLine.
More information about the hotspot-dev