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

[sangheon.kim]

Hi David,

Thank you for reviewing this.
I added my answers below.

On 07/16/2015 08:48 PM, David Holmes wrote:
> Hi Sangheon,
>
> On 17/07/2015 7:52 AM, sangheon.kim wrote:
>> Hi Coleen,
>>
>> Thank you for reviewing this.
>>
>> On 07/16/2015 01:05 PM, Coleen Phillimore wrote:
>>>
>>> http://cr.openjdk.java.net/~sangheki/8130459/webrev.00/src/share/vm/runtime/commandLineFlagConstraintList.cpp.udiff.html 
>>>
>>>
>>>
>>> This
>>>
>>> +CommandLineFlagConstraint::ConstraintType
>>> CommandLineFlagConstraint::change_to_enum(const char* type) {
>>>
>>> function will return the enum if there is junk at the end of the
>>> string.  Is that expected?
>> No.
>> Now I'm thinking like below:
>> if (strncmp(type, afterParse_str, 10) == 0 && type[10] == '\0') {
>
> That seems fine to me.
>
> Overall this set of changes looks good to me, though I have a couple 
> of nits:
>
> 1. It would be nice if the constraint type could default to AnyTime, 
> rather than having to be explicitly declared - though I understand if 
> the macros make that impractical.
I thought to have explicit declaration would be better.
And also I didn't want to adding more complexity into our macro. :)

>
> 2. Why do you use AfterParse when it is really "after ergo" ? I can 
> imagine you might need to distinguish the two in the future.
I didn't think of that.
Do you prefer 'AfterErgo'?

>
> 3. check_ranges_and_constraints_of_after_parse seems to have three jobs:
> - a) possibly print ranges and constraints
> - b) check ranges
> - c) check constraints
> Given you are introducing multiple constraint checking phases I think 
> it would be better to factor out the constraint part separately, 
I agree to separating constraints part.

> then you would not need check_constraints_of_after_parse or 
> check_constraints_of_after_memory_init, as you could simply call eg.:
>
> check_constraints(CommandLineFlagConstraint::AfterMemoryInit);
I also thought about this approach but I thought accessing only via 
CommandLineFlags class would be better rather than including 
commandLineFlagConstraintList.hpp file.
But I don't have strong opinion on this.

>
> and have it set _validating_type.
>
> Also "check constraints of after" is not grammatically well formed.
These names mean 'check constraint of "AnyTime", "AfterParse", 
"AfterMemoryInit"'. :)

David, I will prepare updated webrev after your answer.

Thanks,
Sangheon


>
> Thanks,
> David
>
>> Or do you have any recommendation?
>> Let me send a revised webrev after hearing your opinion.
>>
>>>
>>> http://cr.openjdk.java.net/~sangheki/8130459/webrev.00/src/share/vm/runtime/commandLineFlagConstraintList.hpp.udiff.html 
>>>
>>>
>>>
>>> Generally the nonstatic data members of a class are written before the
>>> functions (at the top of the class declaration).  You may have to put
>>> these after the enum declaration, but not at the bottom.
>> Okay, I will fix it.
>>
>>>
>>> I like this change.  I think it looks good.
>> Thank you!
>>
>> Sangheon
>>
>>>
>>> Coleen
>>>
>>>
>>> On 7/15/15 3: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
>>>
>>