RFR(S): 8142341: GC: current flags need ranges to be implemented
sangheon.kim
sangheon.kim at oracle.com
Fri Dec 4 20:01:25 UTC 2015
Hi Tom,
On 12/04/2015 11:17 AM, Tom Benson wrote:
> Hi Sangheon,
> I'm also also OK with the change, but I found the wording of this
> comment a little confusing, because 'value' is also the name of the
> argument which is the HeapBaseMinAddress being checked:
>
> 554 // But below check is okay as the wrong value means bigger than
> the value should be.
>
> How about something like: If an overflow happened in
> Arguments::set_heap_size(), MaxHeapSize will have too large a value.
> Check for this by ensuring that MaxHeapSize plus the requested min
> base address still fit within max_uintx.
> ?
Your suggestion seems much cleaner. I will use this!
Do you need a new webrev for this?
Thanks,
Sangheon
> Tnx,
> Tom
>
>
> On 12/4/2015 1:07 PM, 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.
>>
>> 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