RFR 8130459(M): Add additional validation after heap creation
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Mon Jul 20 19:05:54 UTC 2015
Sangheon, please see my comment inline.
On 20.07.2015 20:24, sangheon.kim wrote:
> Hi Dmitry,
>
> Thanks for reviewing this.
> I added inline answer.
>
> On 07/20/2015 10:09 AM, Dmitry Dmitriev wrote:
>> Hello Sangheon,
>>
>> I think that you can rid from
>> CommandLineFlagConstraint::change_to_enum function by using
>> EMIT_CONSTRAINT_CHECK macro in following form:
>> #define EMIT_CONSTRAINT_CHECK(func,
>> type) , func,
>> CommandLineFlagConstraint::type
> The idea of changing to enum seems nice.
> But with this approach we can't detect typos or wrong types.
This approach allow to detect typos, because code in this case will not
compile. For example if we specify "AtPurse" instead of "AtParse", then
after preprocessor it will result to
"CommandLineFlagConstraint::AtPurse" and this value will not be found in
ConstraintType enum. Or you mean something different?
Thanks,
Dmitry
>
> Thanks,
> Sangheon
>
>
>>
>> And changing emit_constraing_##type functions accordingly. E.g. to
>> void emit_constraint_bool(const char* name,
>> CommandLineFlagConstraintFunc_bool func,
>> CommandLineFlagConstraint::ConstraintType type) {
>> CommandLineFlagConstraintList::add(new
>> CommandLineFlagConstraint_bool(name, func, type));
>> }
>>
>> But probably I miss something and not take something into account, so
>> fell free to correct me. Thanks!
>>
>> Dmitry
>>
>> On 18.07.2015 8:11, 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.
>>>
>>> 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
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list