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

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Fri Dec 4 22:41:18 UTC 2015 

Den 4/12/15 kl. 20:59, skrev sangheon.kim:
> Hi Jesper,
>
> Thank you for reviewing this.
>
> On 12/04/2015 10:07 AM, Jesper Wilhelmsson wrote:
>> 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.
> You are right.
> However let me address from a separate CR as you said old lines also don't have
> a space.

I'm fine with that.
Ship it!
/Jesper

>
>>
>> Since you filed the other bug regarding the overflow I think this patch is
>> good to go.
> Thanks!
>
> Sangheon
>
>
>> /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