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

[sangheon.kim]

Hi Gerard,

Okay, thanks for the review.

Sangheon


On 07/16/2015 12:43 PM, gerard ziemski wrote:
> 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
>>>>
>>>>
>>>
>>
>>
>>
>