RFR 8078555(M): GC: implement ranges (optionally constraints) for those flags that have them missing

[sangheon.kim]

Hi Kim,

Here's webrev.03 which includes your comment for MarkStackSize 
constraint function.

http://cr.openjdk.java.net/~sangheki/8078555/webrev.03
http://cr.openjdk.java.net/~sangheki/8078555/webrev.03_to_02/

And all your comments will be managed by 
https://bugs.openjdk.java.net/browse/JDK-8134348 .

Thanks,
Sangheon


On 08/24/2015 10:51 AM, Kim Barrett wrote:
> On Aug 24, 2015, at 11:36 AM, sangheon.kim <sangheon.kim at oracle.com> wrote:
>> Hi Kim,
>>
>> On 08/21/2015 02:34 PM, Kim Barrett wrote:
>>> Comments below are for:
>>>
>>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.01/
>>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.01_to_00/
>>>
>>> No additional comments for:
>>>
>>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.02/
>>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.02_to_01
>>>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>>
>>> General comment - all the conditionalizations on INCLUDE_ALL_GCS
>>> seriously uglifies this code.  We should be conditionalizing for that,
>>> but I wonder if there's a better way.  Most of the conditionalizations
>>> are for effectively the entire constraint function.  Rather than that,
>>> could we arrange for the relevant constraint functions to be
>>> unreferenced when not needed, and then just have a block
>>> conditionalization around the whole set of functions?  One way to do
>>> this might be to add a NOT_INCLUDE_ALL_GCS macro and wrap the
>>> "constraint(...)" forms with that new macro where appropriate.
>>>
>>> This probably should be addressed under a separate RFE though.
>>>
>>> If I'd realized how ugly it would make the code, I probably wouldn't
>>> have suggested doing the conditionalization now.  But now that it's
>>> here, I'm not sure its worth backing out.
>> I agree with you.
>> How about leave as is and handle from a separate RFE?
> OK.
>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>>    63 Flag::Error ParallelGCThreadsConstraintFunc(uint value, bool verbose) {
>>> [removed the following]
>>>    71   // G1, CMS and Parallel GC modes disallows ParallelGCThreads=0
>>>    72   if ((UseG1GC || UseConcMarkSweepGC || UseParallelGC) && (value == 0)) {
>>>    73     CommandLineError::print(verbose, "ParallelGCThreads cannot be zero\n");
>>>    74     return Flag::VIOLATES_CONSTRAINT;
>>>    75   }
>>>
>>> Why was this removed?
>> Disallowing ParallelGCThreads=0 is already checked before constraint check.
>> (By Arguments::set_parnew_gc_flags(), set_parallel_gc_flags() and set_g1_gc_flags() )
> Ah, missed that.  OK.
>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/runtime/commandLineFlagConstraintsGC.cpp
>>> #if INCLUDE_ALL_GCS
>>>   271 Flag::Error G1RSetRegionEntriesConstraintFunc(intx value, bool verbose) {
>>>   287 Flag::Error G1RSetSparseRegionEntriesConstraintFunc(intx value, bool verbose) {
>>>   303 Flag::Error G1YoungSurvRateNumRegionsSummaryConstraintFunc(intx value, bool verbose) {
>>>   317 Flag::Error G1HeapRegionSizeConstraintFunc(size_t value, bool verbose) {
>>>   332 Flag::Error G1NewSizePercentConstraintFunc(uintx value, bool verbose) {
>>>   346 Flag::Error G1MaxNewSizePercentConstraintFunc(uintx value, bool verbose) {
>>> #endif
>>>
>>> [pre-existing]
>>>
>>> Shouldn't these G1-specific constraint functions (whose options are
>>> declared in g1_globals.hpp) be declared and defined in G1-specific
>>> files?
>>>
>>> This should be handled as a separate issue.
>> I'm not sure whether is it worth or not.
>> But as I don't have a strong opinion on this, let me add this at follow-up issue.
> OK.
>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/runtime/globals.hpp
>>> 1857   product(size_t, MarkStackSize, NOT_LP64(32*K) LP64_ONLY(4*M),             \
>>> 1858           "Size of marking stack")                                          \
>>> 1859           range(0, MarkStackSizeMax)                                        \
>>>
>>> I raised an issue here previously, and re-ordering the declarations in
>>> the macros doesn't address the problem.
>>>
>>> The value of MarkStackSizeMax will be read at the time of the call to
>>> CommandLineFlagRangeList::init().  This is prior to any command line
>>> option processing for a value for MarkStackSizeMax.  So the range
>>> won't reflect command line options.
>>>
>>> Option values cannot be used in the computation of ranges.
>>> Dependencies between options need to be handled via constraint
>>> functions.
>> You are right.
>> I should think carefully when you left a comment before.
>> Fixed.
> Good.
>
>> Let me send a webrev after hearing your opinion.
> OK.
>