RFR 8130459(M): Add additional validation after heap creation
David Holmes
david.holmes at oracle.com
Fri Jul 17 03:48:10 UTC 2015
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.
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.
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, 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);
and have it set _validating_type.
Also "check constraints of after" is not grammatically well formed.
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