RFR 8130459(M): Add additional validation after heap creation
Dmitry Dmitriev
dmitry.dmitriev at oracle.com
Wed Jul 22 11:12:04 UTC 2015
Hello Sangheon,
Thank you! Looks good to me. Not a reviewer.
Dmitry
----- Original Message -----
From: sangheon.kim at oracle.com
To: dmitry.dmitriev at oracle.com, hotspot-dev at openjdk.java.net
Sent: Monday, July 20, 2015 10:48:15 PM GMT +03:00 Iraq
Subject: Re: RFR 8130459(M): Add additional validation after heap creation
Hi Dmitry,
On 07/20/2015 12:05 PM, Dmitry Dmitriev wrote:
> 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?
You are right! Sorry, I didn't check yours correctly.
As 'type' belongs to 'CommandLineFlagConstraint' enum, your suggestion
will work correctly including detection of typos.
Here's webrev.03 including your suggestion of 'removing change_to_enum()'.
http://cr.openjdk.java.net/~sangheki/8130459/webrev.03/
Thanks,
Sangheon
>
> 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