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

[David Holmes]

On 17/07/2015 2:26 PM, sangheon.kim wrote:
> 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'?

That works for me.

>>
>> 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.

You could have the wrappers on CommandlineFlags class to avoid the 
additional includes - though I'm not sure I'd be that worried about it. 
But you could still have those wrappers just call check_constraints and 
have check_constraints set the validating type.

So when do Anytime constraints get checked? I seem to be missing that 
part ??

>>
>> 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"'. :)

Ah I see I was parsing it wrong. Okay.

Thanks,
David

>
> 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
>>>>
>>>
>