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

[David Holmes]

On 21/07/2015 3:07 AM, sangheon.kim wrote:
> Hi David,
>
> On 07/20/2015 01:58 AM, David Holmes wrote:
>> 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.
> Okay I will fix these tests before pushing this change.
> Or wait until 8112746 pushed.
>
> BTW, may I ask the reason?
> I thought tests which run during JPRT are used from the source code
> which I submitted.

When you submit a push job it uses the source code changes that you have 
_committed_. If you don't commit the test fixes your push job will not 
have them and your push will fail. A push job basically clones the repo 
the push is intended for and then imports the changesets coming from 
your repo.

Cheers,
David


> Thanks,
> Sangheon
>
>
>>
>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>