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

[Kim Barrett]

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.