RFR(S): 8142341: GC: current flags need ranges to be implemented

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Fri Dec 4 18:07:39 UTC 2015 

Looks good!

I would prefer if there was a space after the comma in the new constraint lines 
in globals.hpp. I see that there are no spaces in the old lines as well. I you 
feel like it this could be cleaned up.

Since you filed the other bug regarding the overflow I think this patch is good 
to go.
/Jesper

Den 3/12/15 kl. 10:08, skrev sangheon.kim:
> 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