RFR(S): 8142341: GC: current flags need ranges to be implemented
sangheon.kim
sangheon.kim at oracle.com
Thu Dec 3 09:08:22 UTC 2015
Hi all,
Here is next webrev.
http://cr.openjdk.java.net/~sangheki/8142341/webrev.02/
http://cr.openjdk.java.net/~sangheki/8142341/webrev.02_to_01/
Testing: JPRT and TestOptionsWithRanges.java on machines that have over
120GB memory.
*Summary
1. Enhanced the constraint function for HeapBaseMinAddress: (Tom)
1) It will check an overflow the addition of MaxHeapSize and
HeapBaseMinAddress. And the overflow only happens when MaxHeapSize is
default and using compressed oop. Since MaxHeapSize is updated after
ergo, FLAG_IS_ERGO(MaxHeapSize) is necessary at the constraint function.
2) As HeapBaseMinAddress has 'AfterErgo' tag, the constraint function
will be called after the overflow happens at Arguments::set_heap_size().
However it is okay as we can detect the overflow before actual use of
creating virtual space. And the problem of the overflow is setting
MaxHeapSize with bigger value that it should be.
3) Changed the order of HeapBaseMinAddress to locate later than
MaxHeapSize from globals.hpp as HeapBaseMinAddress refers MaxHeapSize.
2. Added missing single quote in commandLineFlagConstraintsGC.cpp at
line 510. (Tom)
3. Changed test to add dependency for NewSizeThreadIncrease.
Thanks,
Sangheon
On 12/01/2015 03:55 PM, sangheon.kim wrote:
> Hi Tom,
>
> Thank you for this kind analysis!
>
>
> On 12/01/2015 10:51 AM, Tom Benson wrote:
>> Hi Sangheon,
>>
>> On 11/30/2015 6:51 PM, sangheon.kim wrote:
>>> Hi Tom,
>>>
>>> Thank you for reviewing this!
>>>
>>> On 11/30/2015 02:46 PM, Tom Benson wrote:
>>>> Hi Sangheon,
>>>>
>>>> I think there's a problem with using the same routine to check
>>>> HeapBaseMinAddress as is used for checking heap size in
>>>> commandLineFlagConstraintsGC.cpp. I tried running this, to make
>>>> sure I understood what it was trying to do. When I specified
>>>> something too large for HeapBaseMinAddress, the check reported that
>>>> the value must be greater than or equal to maximum value N.
>>> Sorry Tom, I can't understand.
>>> Currently we print the error message if its value is too large like,
>>> "must be less than or equal to aligned maximum value (xxx)".
>>> Do you mean it should be 'address' instead of 'value'?
>>
>> What I meant was that I got an internal error when I used the value
>> the message recommended, so perhaps the value should be further
>> constrained.
> I see.
>
>>
>>> I thought it is okay.
>>>
>>>> Re-running with that (still huge) value, I get a fatal internal
>>>> error out of virtualspace.cpp. (In the debug version, an assertion
>>>> in universe.cpp triggers before reaching this point). EG:
>>> I am trying to reproduce this assert but I can't. Do you have more
>>> information to reproduce this?
>>> I did -version and running GCOld to trigger GC.
>>>
>>> java -XX:HeapBaseMinAddress=18446744073642442752 -version
>>> java -XX:HeapBaseMinAddress=18446744073642442752 GCOld 20 200 10 100
>>> 5000
>>>
>>
>> You probably didn't hit the problem because you're running on a
>> smaller memory system than I am. On the system I'm on, the
>> (computed) max heap size is 32G. And if you specify an Xmx, you
>> don't go through the code path that causes it.
> Okay, my machine is 32G but when I test on bigger memory machine, I
> could see the assert. Thank you.
>
>>
>> Here's the root of the problem. In Arguments::set_heap_size(), we
>> have this code:
>>
>> if (FLAG_IS_DEFAULT(MaxHeapSize)) {
>> . . .
>> if (UseCompressedOops) {
>> // Limit the heap size to the maximum possible when using
>> compressed oops
>> julong max_coop_heap = (julong)max_heap_for_compressed_oops();
>> . . .
>> if (HeapBaseMinAddress + MaxHeapSize < max_coop_heap) {
>> <======== *** PROBLEM IS HERE ***
>> // Heap should be above HeapBaseMinAddress to get zero based
>> compressed oops
>> // but it should be not less than default MaxHeapSize.
>> max_coop_heap -= HeapBaseMinAddress;
>> }
>> reasonable_max = MIN2(reasonable_max, max_coop_heap);
>> }
>>
>> HeapBaseMinAddress is 64-bit unsigned, and in this case is
>> 0xfffffffffc000000 (the 18446744073642442752 from the command line).
>> MaxHeapSize is 0x7CCCCC8 and max_coop_heap is 0x7FE000000. The
>> addition of HeapBaseMinAddress and MaxHeapSize overflows to
>> 0x3ccccc8. Since this is less than 0x7FE000000, we subtract the
>> 0xfffffffffc000000 from max_coop_heap, getting the result
>> 0x802000000. reasonable_max is 0x800000000, so it stays at that
>> value after the MIN2, rather than being reduced as it should be.
>>
>> Changing the conditional to also check for overflow avoids the
>> problem. But of course you don't get the requested
>> HeapBaseMinAddress anyway, since it's too high.
>>
>> So another approach would be to change your constraint to ensure that
>> (HeapBaseMinAddress plus MaxHeapSize) won't overflow. I don't think
>> there's any point in allowing a HeapBaseMinAddress so high that it
>> would.
> Okay, I'm thinking about this.
> I will post a new webrev after testing.
>
> * I also read your other email that fixing constraint function makes
> sense.
> ** I already fixed your previous comment of extraneous single quote in
> commandLineFlagConstraintsGC.cpp at line 510.
>
> Thanks,
> Sangheon
>
>
>>
>> Tom
>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>>
>>>> $ $JAVA_HOME/bin/java -XX:HeapBaseMinAddress=0xffffffffffffffff
>>>> -version
>>>> HeapBaseMinAddress (18446744073709551615) must be less than or
>>>> equal to aligned maximum value (18446744073642442752)
>>>> Error: Could not create the Java Virtual Machine.
>>>> Error: A fatal exception has occurred. Program will exit.
>>>>
>>>> $ $JAVA_HOME/bin/java -XX:HeapBaseMinAddress=18446744073642442752
>>>> -version
>>>> #
>>>> # A fatal error has been detected by the Java Runtime Environment:
>>>> #
>>>> # Internal Error (virtualspace.cpp:456), pid=10359, tid=10360
>>>> # guarantee(size + noaccess_prefix_size(alignment) <=
>>>> OopEncodingHeapMax) failed: can not allocate compressed oop heap
>>>> for this size
>>>> #
>>>>
>>>> Perhaps you should check only the alignment in the constraint code,
>>>> without checking the range, because I'm not sure you have the final
>>>> heap size at that point. Then maybe the heap reservation code
>>>> should report this as a non-internal error, at the point where the
>>>> assertion occurs, if the user specified a base address.
>>>>
>>>> There's also an extraneous single quote in
>>>> commandLineFlagConstraintsGC.cpp in the comment at line 510.
>>>>
>>>> Tom
>>>>
>>>>
>>>> Date: Tue, 24 Nov 2015 06:37:41 -0800
>>>> From: "sangheon.kim"<sangheon.kim at oracle.com>
>>>> To: Dmitry Dmitriev<dmitry.dmitriev at oracle.com>,
>>>> "hotspot-dev at openjdk.java.net Developers"
>>>> <hotspot-dev at openjdk.java.net>
>>>> Subject: Re: RFR(S): 8142341: GC: current flags need ranges to be
>>>> implemented
>>>> Message-ID:<56547635.9060808 at oracle.com>
>>>> Content-Type: text/plain; charset=utf-8; format=flowed
>>>>
>>>> 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