Revision1: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments

[David Holmes]

On 27/05/2015 3:15 AM, Gerard Ziemski wrote:
> Thank you David for feedback. Please see my answers inline:
>
>> -------- Forwarded Message --------
>> Subject:	Re: Revision1: Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments
>> Date:	Tue, 26 May 2015 18:10:14 +1000
>> From:	David Holmes<david.holmes at oracle.com>
>> Organization:	Oracle Corporation
>> To:	Gerard Ziemski<gerard.ziemski at oracle.com>,hotspot-dev at openjdk.java.net  Developers<hotspot-dev at openjdk.java.net>, Coleen Phillimore<coleen.phillimore at oracle.com>, Dmitry Dmitriev<dmitry.dmitriev at oracle.com>
>> CC:	david.therkelsen at oracle.com, Thomas Stüfe<thomas.stuefe at gmail.com>, sangheon.kim<sangheon.kim at oracle.com>
>>
>> Hi Gerard,
>>
>> Progress review so I can send this out tonight - I still have to
>> complete the review and double-check the responses to my previous comments.
>>
>> In globals.hpp, looking at all the "stack parameters" I expect to see a
>> constraint function specified somewhere, but there isn't one. So now I'm
>> a bit confused about how constraint functions are specified and used. If
>> there has to be a relationship maintained between A, B and C, is the
>> constraint function specified for all of them or none of them and simply
>> executed as post argument processing step? Can you elaborate on when
>> constraint functions may be used, and must be used, and how they are
>> processed?
>>
> Constraints were not meant as a framework that imposes restrictions as to when and how to be used. It’s a helper framework that makes it easy for a developer to implement the kind of a constraint that a particular flag(s) demands. The decision as to what goes into it is left to the engineer responsible for a particular flag. The process of implementing constraints and ranges is still ongoing for many of the flags, and there are 3 subtasks tracking the issue. This webrev covers the introduction of the range/constraint framework and a subset of ranges/constraints implemented for those flags for which I was able to find existing ad hoc code or comments describing them.

That's not really answering the question. Let me assume from this that 
the stack parameters have not been updated yet - fine. Now lets suppose 
that I want to update them using a constraint function. How do I do 
that? Do I specify the constraint function on each argument involved in 
the constraint? When will the constraint function be executed?

Thanks,
David

>
>> A few minor specific comments below:
>>
>> src/share/vm/c1/c1_globals.hpp
>>
>> Minor nit: Could you change:
>>
>>    range(1, NOT_LP64(K) LP64_ONLY(32*K))
>>
>> to
>>
>>    range(1, NOT_LP64(1*K) LP64_ONLY(32*K))
>>
>> or even:
>>
>>    range(1, NOT_LP64(1) LP64_ONLY(32) *K)
>>
>> I find the (K) by itself a little odd-looking.
>>
> Done.
>
>
>> ---
>>
>> src/share/vm/runtime/globals.cpp
>>
>> +       if (withComments) {
>> + #ifndef PRODUCT
>> +         st->print("%s", _doc);
>> + #endif
>> +       }
>>
>> The ifdef should be around the whole if block (as it should for the
>> existing code just before this change).
>
> Done.
>
>
>> ---
>>
>> src/share/vm/runtime/globals.hpp
>>
>> range(1, NOT_LP64(K) LP64_ONLY(M))
>>
>> 1*K and 1*M please.
>
> Done.
>
>
>> 1328   /* 8K is well beyond the reasonable HW cache line size, even with
>> the   */\
>>
>> delete the end 'the'
>>
>> 1329   /* aggressive prefetching, while still leaving the room for
>> segregating */\
>>
>> delete 'the’.
>
> Done.
>
>
>> Nit:
>>
>> 1981           range(ReferenceProcessor::DiscoveryPolicyMin,
>>           \
>> 1982           ReferenceProcessor::DiscoveryPolicyMax)
>>           \
>>
>> 1982 should be indented so the Reference's line up.
>
> Done.
>
>
>> 2588
>> range((intx)Arguments::get_min_number_of_compiler_threads(),      \
>> 2589                 max_jint)
>>           \
>>
>> Seems odd for a range to be expressed this way - seems more like a
>> constraint. And get_min_number_of_compiler_threads doesn't really seem
>> like an API for Arguments.
>>
> All ranges could be expressed as constraints. Ranges are just “trivial” constraint, which is most of them, and are supposed to be easy to use by the developer. get_min_number_of_compiler_threads() in this context of expressing a range is “constant”. I need that value; it used to be accessed from Arguments internally, so it did not need to be exposed before, but with the range/constraints factored out, I need to be able to get at the value.
>
> I will be persenting webrev 2 shortly after I do build and some testing.
>
>
> cheers
>