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

[gerard ziemski]

hi Sangheon,

I have no further comments, please consider this reviewed with small "r".


cheers

On 07/15/2015 08:46 PM, sangheon.kim wrote:
> Hi Gerard,
>
> 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.
> Thank you!
>
>>
>> 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 
>> "find_matching_current_validation_type()"
> 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 
> _validating_type.
>
> http://cr.openjdk.java.net/~sangheki/8130459/webrev.01/
>
> Thanks,
> Sangheon
>
>
>>
>>
>> cheers
>>
>>
>>
>> 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 
>>> constraints.
>>> 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 
>>> initialization.
>>> 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 
>>> CommandLineFlags::finishedInitializing().
>>> - AfterParse: constraint functions which use 
>>> CommandLineFlags::finishedInitializing().
>>> - 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.
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>
>
>
>