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

Mon Jul 20 08:58:16 UTC 2015

Hi Sangheon,

On 18/07/2015 3:11 PM, sangheon.kim wrote:
> Hi David and Coleen,
>
> Okay, I totally agree to your opinions.
> And here is webrev v02.
> http://cr.openjdk.java.net/~sangheki/8130459/webrev.02/
>
> - String compare to avoid junk at the end of the string at
> CommandLineFlagConstraint::change_to_enum. (Coleen)
> - Moved nonstatic data members to locate before the functions. (Coleen)
> - Renamed 'Anytime => AtParse' and ' AfterParse => AfterErgo'. (David
> and Coleen)
> - Separated 'CommandLineFlags::check_all_ranges_and_constraints' to
> 'check_ranges', 'check_constraints_of_after_ergo'. (David)
>    : This will change the behavior.
>    If we check ranges and constraints separately and stop when one of
> them fails(at thread.cpp), 2 tests(
> test/runtime/CompressedOops/ObjectAlignment.java,
> test/runtime/contended/Options.java ) will fail. And these will be
> covered by JDK-8112746
> <https://bugs.openjdk.java.net/browse/JDK-8112746> (Followup to
> JDK-8059557).
>    I just added patched test cases temporarily to pass JPRT.

You have to fix the tests before this can be pushed else you won't get 
the push through JPRT.

David
-----

> Tested: JPRT, UTE(vm.quick-pcl), test/runtime/CommandLine/*
>
> Thanks,
> Sangheon
>
>
> On 07/17/2015 05:23 AM, Coleen Phillimore wrote:
>>
>> I like David's new name suggestions as well.  Anytime => AtParse and
>> AfterParse => AfterErgo look better to me.
>> That was the only thing I was unsure about in my review of this code
>> and didn't have a better suggestion.  The rest looks good to me.  I
>> prefer _not_ having default argument of AtParse and not adding that
>> complication to the macros.  It seems that the constraint writer
>> should specify phase when to check the argument.
>>
>> Thanks,
>> Coleen
>>
>> On 7/17/15 2:42 AM, David Holmes wrote:
>>> On 17/07/2015 4:19 PM, sangheon.kim wrote:
>>>> On 07/16/2015 09:49 PM, David Holmes wrote:
>>>>> 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.
>>>> Okay.
>>>>
>>>>>
>>>>>>>
>>>>>>> 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.
>>>> I meant constraint type enum is at 'commandLineFlagConstraintList.hpp
>>>> and currently 'globals.hpp' doesn't include headers except 'debug.hpp'
>>>> and variant of 'globals_xxx.hpp'.
>>>> So I didn't want to add 'commandLineFlagConstraintList.hpp on that.
>>>> And also I have to add 'commandLineFlagConstraintList.hpp' at
>>>> thread.cpp
>>>> and universe.cpp to use as a parameter.
>>>>
>>>> How about like below?
>>>> - bool check_ranges();
>>>> - bool check_constraints_of_after_ergo();
>>>> - bool check_constraints_of_after_memory_init();
>>>
>>> Sure
>>>
>>>>>
>>>>> So when do Anytime constraints get checked? I seem to be missing that
>>>>> part ??
>>>> Anytime is checked during argument processing.
>>>> e.g. checking double..
>>>> Arguments::parse_argument -> set_fp_numeric_flag ->
>>>> CommandLineFlags::doubleAtPut ->
>>>> apply_constraint_and_check_range_double.
>>>
>>> Okay - it's buried :) In that case can I suggest using AtParse
>>> instead of Anytime - as it isn't really any time, it will happen when
>>> parsing that argument. Not a big deal if others object.
>>>
>>> Thanks,
>>> David
>>>
>>>>>
>>>>>>>
>>>>>>> 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.
>>>> To avoid header file matter, these names became a little longer.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>
>>>>>
>>>>> 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
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>