RFR(S): 8142341: GC: current flags need ranges to be implemented
sangheon.kim
sangheon.kim at oracle.com
Tue Dec 1 01:33:41 UTC 2015
Hi Gerard again,
On 11/30/2015 03:41 PM, sangheon.kim wrote:
> Hi Gerard,
>
> Thank you for reviewing this!
> I was waiting your review.
>
> On 11/30/2015 02:39 PM, gerard ziemski wrote:
>> 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
> Right.
> There are some other cases like you mentioned.
> Let me address this issue separately.
>
>>
>>
>> -------------
>> #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) \
> As I shortly explained my RFR, this value is used to set a function
> which has an input parameter of 'long' type.
> And I think it would be a good reason to narrow its range.
>
>>
>>
>> -------------
>> #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) ?
> When we reach to that line, InitialSurvivorRatio will be changed to
> '3' for min by line 58. generationSizer.cpp
> GenerationSizer::initialize_flags() {
> ..
> if (InitialSurvivorRatio < 3) {
> ..
> }
>
>>
>> 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) ?
> NumberOfGCLogFiles==0 is default value for disabling rotation.
> And before reaching that line, we already filtered NumberOfGCLogFiles
> <= 1 cases.
>
>>
>> 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;
>> }
> Right. We have more cases including this.
> David Lindholm filed a CR for moving JDK-8133649.
>
>>
>> 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?)
> You are right.
> We don't have any problem without constraint function but it would be
> better to have one.
> I will post next webrev soon with this constraint function.
Looking at the code again, it is hard to add constraint function for
this flag.
When we run the constraint function('threads_count' is zero at this
time), we don't know how many non-daemon threads will be running.
i.e. 'size_t thread_increase_size = threads_count *
NewSizeThreadIncrease; ' is executed after GC is happened and the flag
validation is occurred much earlier.
And as we are using both MIN/MAX macro, I think there will be no value
range problem with this flag.
What do you think about this?
Thanks,
Sangheon
>
> Thanks,
> Sangheon
>
>
>>
>> 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