RFR(S): 8142341: GC: current flags need ranges to be implemented
sangheon.kim
sangheon.kim at oracle.com
Mon Nov 30 23:41:25 UTC 2015
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.
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