RFR(S): 8142341: GC: current flags need ranges to be implemented
gerard ziemski
gerard.ziemski at oracle.com
Mon Nov 30 22:39:44 UTC 2015
hi Sangheon,
I found a few issues:
-------------
#1 An already existing issue, but there are many candidate flags that could have their types changed to "size_t", that
currently are defined as some other type (usually uintx) ex: ParGCDesiredObjsFromOverflowList, CMSOldPLABReactivityFactor
-------------
#2 Why isn't the range here "range(min_intx, max_intx)" ?
src/share/vm/runtime/globals.hpp lines:
1985 manageable(intx, CMSWaitDuration, 2000, \
1986 "Time in milliseconds that CMS thread waits for young GC") \
1987 range(min_jint, max_jint) \
-------------
#3 The min in the range must not be 0, because we use it as denominator:
size_t survivor_size = size / InitialSurvivorRatio;
so the range should be range(1, max_uintx) ?
Lines:
src/share/vm/runtime/globals.hpp lines:
2347 "Initial ratio of young generation/survivor space size") \
2348 range(0, max_uintx) \
-------------
#4 The min in the range must not be 0, because we do:
if (_cur_file_num > NumberOfGCLogFiles - 1) _cur_file_num = 0;
so the range should be range(1, max_uintx) ?
src/share/vm/runtime/globals.hpp lines:
2599 product(uintx, NumberOfGCLogFiles, 0, \
2600 "Number of gclog files in rotation " \
2601 "(default: 0, no rotation)") \
2602 range(0, max_uintx) \
-------------
#5 TLABRefillWasteFraction should have a constraint moved from Arguments::check_vm_args_consistency to
src/share/vm/runtime/commandLineFlagConstraintsGC ?
// Check the consistency of vm_init_args
bool Arguments::check_vm_args_consistency() {
// Method for adding checks for flag consistency.
// The intent is to warn the user of all possible conflicts,
// before returning an error.
// Note: Needs platform-dependent factoring.
bool status = true;
if (TLABRefillWasteFraction == 0) {
jio_fprintf(defaultStream::error_stream(),
"TLABRefillWasteFraction should be a denominator, "
"not " SIZE_FORMAT "\n",
TLABRefillWasteFraction);
status = false;
}
src/share/vm/runtime/globals.hpp lines:
3419 product(uintx, TLABRefillWasteFraction, 64, \
3420 "Maximum TLAB waste at a refill (internal fragmentation)") \
3421 range(1, max_juint) \
-------------
#6 We multiply NewSizeThreadIncrease later, like
size_t thread_increase_size = threads_count * NewSizeThreadIncrease;
so the range's max needs to be smaller than max_uintx by some large enough constant (MAX thread count?)
src/share/vm/runtime/globals.hpp lines:
3437 product_pd(size_t, NewSizeThreadIncrease, \
3438 "Additional size added to desired new generation size per " \
3439 "non-daemon thread (in bytes)") \
3440 range(0, max_uintx) \
cheers
On 11/25/2015 05:00 PM, sangheon.kim wrote:
> Hi all,
>
> Here's updated webrev which reflects changes by "JDK-8143038: [TESTBUG] TestOptionsWithRanges: allow excluding only a
> subset of tested values specified for a flag".
>
> The only updated file is test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java.
> Changed from "allOptionsAsMap.remove("flag name")" to "excludeTestMaxRange("flag name")".
>
> FYI, JDK-8143038 introduced separated exclude methods of "excludeTest, excludeTestMaxRange and excludeTestMinRange".
>
> webrev:
> http://cr.openjdk.java.net/~sangheki/8142341/webrev.01/
> http://cr.openjdk.java.net/~sangheki/8142341/webrev.01_to_00/
>
> Thanks,
> Sangheon
>
>
> On 11/24/2015 06:37 AM, sangheon.kim wrote:
>> Hi Dmitry,
>>
>> Thank you for the review!
>> Sure I will update my patch related to your enhancement.
>>
>> Thanks,
>> Sangheon
>>
>>
>> On 11/24/2015 06:30 AM, Dmitry Dmitriev wrote:
>>> 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
>>>>
>>>>
>>>>
>>>
>>
>
>
More information about the hotspot-dev
mailing list