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

[sangheon.kim]

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?

>
> ------------------------------------------------------------------------------
> 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() )

>
> ------------------------------------------------------------------------------
> 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.

>
> ------------------------------------------------------------------------------
> 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.

Let me send a webrev after hearing your opinion.

Thanks,
Sangheon

>
> ------------------------------------------------------------------------------
>
>