RFR(S): 8142341: GC: current flags need ranges to be implemented

[Dmitry Dmitriev]

Hi Sangheon,

Looks good to me! Thank you for fixing that. I hope that enhancement 
will be pushed today(currently in JPRT queue), so please update your 
patch after that!

Thanks,
Dmitry

On 21.11.2015 0:04, sangheon.kim wrote:
> Hi all,
>
> Could I have a couple of reviews for this change to add explicit 
> 'range' for flags?
>
> Previously, we added 'range' when it is needed to avoid assert/crash 
> but now explicit 'range' are needed for all flags.
> All flags should have 'range' or 'constraint' at least.
>
> CR: https://bugs.openjdk.java.net/browse/JDK-8142341
> Webrev: http://cr.openjdk.java.net/~sangheki/8142341/webrev.00
> Testing: JPRT, RBT 
> (hotspot/test/:hotspot_all,testlist,noncolo.testlist 
> --add-tonga-keyword quick), hotspot/test/runtime/CommandLine for embedded
>
> * Summary
> 1. Added 3 new constraint functions.
>    1) HeapBaseMinAddress: Added to avoid an overflow after align up 
> and this flag makes hang up without constraint function. (newly added 
> as a part of GC work)
>    2) TLABWasteIncrement: Without this constraint function we don't 
> have problems (even many GCs happen). But added to avoid an overflow 
> at ThreadLocalAllocBuffer::record_slow_allocation(). There are also 
> separate CR for this overflow ( JDK-8143352 ).
>    3) NUMAInterleaveGranularity: Added to avoid an overflow after 
> align up.
>
> 2. Some flags have narrower range than their type.
>    1) Here's the reason why some flags should have different range. 
> (same order from globals.hpp)
> {flag type} {flag name}: (range), {comment}
> size_t NUMAInterleaveGranularity: (os::vm_allocation_granularity(), 
> max_uintx), there is a comment at numa_interleaving_init() that this 
> flag should be larger than vm_allocation_granularity().
> uintx CMSOldPLABReactivityFactor: (1,max_uintx), couldn't be zero as 
> used for multiplication
> uintx CMS_FLSPadding: (0, max_juint), used to set a function which has 
> 'unsigned int' type input parameter.
> uintx CMS_SweepPadding: (0, max_juint), used to set a function which 
> has 'unsigned int' type input parameter.
> intx CMSWaitDuration: (min_jint, max_jint), used to set a function 
> which has 'long' type input parameter.
> uintx PausePadding: (0, max_juint), used to set a function which has 
> 'unsigned int' type input parameter.
> uintx PromotedPadding: (0, max_juint), used to set a function which 
> has 'unsigned int' type input parameter.
> uintx SurvivorPadding: (0, max_juint), used to set a function which 
> has 'unsigned int' type input parameter.
> uintx AdaptiveSizePolicyCollectionCostMargin: (0, 100), as this flag 
> is divided by 100 I assume this is percentage.
> uintx GCTimeRatio: (0, max_juint), used to set a function which has 
> 'unsigned int' type input parameter.
> intx PrefetchCopyIntervalInBytes: (-1, max_jint)
> intx PrefetchScanIntervalInBytes:  (-1, max_jint)
> intx PrefetchFieldsAhead:  (-1, max_jint), I think these 3 Prefetch* 
> flags should have same upper limit and looking at their related codes 
> 'max_jint' seems appropriate ( no problem with 'max_jint' during 
> testing). However, as Prefetch::read/write() needs 'intx', 'intx' also 
> seems good but we have to fix some codes (maybe including generated 
> codes).
> uintx CPUForCMSThread: (0, max_juint), used to set a function which 
> has 'unsigned int' type input parameter.
> uintx ProcessDistributionStride: (0, max_juint), used to set uint 
> variable and used 'for loop' which has uint increment. i.e. for (uint 
> i=0; i<ProcessDistributionStride; i++)
> uintx CMSCoordinatorYieldSleepCount: (0, max_juint), used at 
> 'increment for loop()' as max value and the increment is uint.
> uintx CMSYieldSleepCount: (0, max_juint),  used at 'increment for 
> loop()' as max value and the increment is uint.
> uintx TLABRefillWasteFraction: (1, max_juint), used as a return value 
> of uint type function and for division. i.e. uint 
> GCTLABConfiguration::tlab_refill_waste_limit()() { return 
> TLABRefillWasteFraction; }
> uintx TLABWasteIncrement: (0, max_jint), type cast to (int)
> intx PrintCMSStatistics: (0,2), flag to choose print mode and we only 
> use " if (a !=0, >0, >1)".
> intx PrintFLSStatistics: (0,2), flag to choose print mode and we only 
> use " if (a !=0, >0, >1)".
> intx PrintFLSCensus: (0,1), flag to choose print mode and we only use 
> " if (a !=0, >0)".
> uintx GCDrainStackTargetSize: (0, max_juint), type cast to 'unsigned int'
>
> (g1_globals.hpp)
> intx G1ConcRefinementThresholdStep: (0, max_jint), used to set (int) 
> type variable.
>
> 3. Excluded flags from TestOptionsWithRanges.java
> 1) Memory and thread related flags: tests for these flags will consume 
> too many resources from the system.
> 2) VMThreadStackSize: range work for this flag is not included in this 
> patch but I faced OOME several times, so excluded.
>
> * I will reflect Dmitry's enhancement(JDK-8143038: [TESTBUG] 
> TestOptionsWithRanges: allow excluding only a subset of tested values 
> specified for a flag) next time.
>
> Thanks,
> Sangheon
>
>
>