RFR(S): 8142341: GC: current flags need ranges to be implemented
sangheon.kim
sangheon.kim at oracle.com
Tue Dec 1 23:55:46 UTC 2015
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