RFR 8078555(M): GC: implement ranges (optionally constraints) for those flags that have them missing

[gerard ziemski]

hi Sangheon,

Looks good.


cheers

On 08/28/2015 01:44 AM, sangheon.kim wrote:
> Hi Jon, Gerard and Derek,
>
> Here's the new webrev which includes:
> 1. @requires vm.gc==xx for 4 test cases. (Derek)
> 2. Change INITIAL_RANGES_SIZE and INITIAL_CONSTRAINTS_SIZE to current actual size. (Gerard)
> 3. Range of G1UpdateBufferSize is 'NOT_LP64(32*M) LP64_ONLY(1*G)'. (Jon)
> 4. 'amount' to 'count' at G1YoungSurvRateNumRegionsSummaryConstraintFunc. (Jon)
> 5. Add more explanation for CheckMaxHeapSizeAndSoftRefLRUPolicyMSPerMB. (Jon)
>
> http://cr.openjdk.java.net/~sangheki/8078555/webrev.04
> http://cr.openjdk.java.net/~sangheki/8078555/webrev.04_to_03
>
> Thanks,
> Sangheon
>
>
> On 08/26/2015 03:30 PM, sangheon.kim wrote:
>>
>>
>> On 08/26/2015 02:56 PM, Jon Masamitsu wrote:
>>>
>>>
>>> On 8/26/2015 1:50 PM, sangheon.kim wrote:
>>>> Hi Jon,
>>>>
>>>> On 08/26/2015 01:34 PM, Jon Masamitsu wrote:
>>>>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.03/src/share/vm/gc/g1/g1_globals.hpp.frames.html
>>>>>
>>>>> How was the 1*G maximum chosen?
>>>>>
>>>>>  114   product(size_t, G1UpdateBufferSize, 256,                                  \
>>>>>  115           "Size of an update buffer")                                       \
>>>>> 116 range(1, 1*G)
>>>>>
>>>>>
>>>>> Maybe this was already discussed?
>>>> You have eagles eye! :)
>>>> Personally I asked to Thomas and his suggestion was 'I think something in the MB range should be "enough" '.
>>>>
>>>> I know that 1G is quite different from MB range but I wanted to find not making problem range.
>>>> One point that would make problem is 'G1CollectedHeap::pending_card_num()'.
>>>> '(buffer_size * buffer_num + extra_cards)' should not overflow and I think 1G would be safe enough.
>>>
>>> 1G seems too large on a 32bit system.  I don't know how large buffer_num
>>> can get but can image it  can get large in some instances.
>>>
>>> Having said that this is not necessarily something that can be fixed at
>>> flag validation time.  This may be one of those cases where the calculated
>>> numbers are too dynamic to catch early.   If you can make the check
>>> different on a 32bit system, 32M might be a better number on a
>>> 32bit system.  Leave it 1G on a 64bit system.
>> Okay, I will change as you mentioned.
>> range(1, NOT_LP64(32*M) LP64_ONLY(1*G))
>>
>> This will be included at next webrev.
>>
>> Thanks,
>> Sangheon
>>
>>
>>>
>>> Jon
>>>
>>>> But I know that it is too large as a buffer size. :(
>>>> Any suggestion for this?
>>>>
>>>> Sangheon
>>>>
>>>>
>>>>>
>>>>> Jon
>>>>>
>>>>> On 8/26/2015 11:51 AM, sangheon.kim wrote:
>>>>>> Hi Derek,
>>>>>>
>>>>>> Thank you for looking at this!
>>>>>>
>>>>>> On 08/26/2015 10:59 AM, Derek White wrote:
>>>>>>> Hi Sangheon,
>>>>>>>
>>>>>>> I haven't reviewed the actual ranges and constraints yet, but one thing you might want to consider:
>>>>>>>
>>>>>>> For the test cases, you may want synchronize the GC specified to ProcessBuilder with the "@requires gc=" tags.
>>>>>>> This prevents the test harness from running G1 tests when the test harness is trying to run CMS test, etc, and
>>>>>>> also avoids potential confusing test failures.
>>>>>>>
>>>>>>>  @requires vm.gc=="G1" | vm.gc=="null"
>>>>>>>  (or specify Parallel GC as needed).
>>>>>> Thank you for the explanation.
>>>>>> I didn't know about this.
>>>>>>
>>>>>> So it seems like vm.gc=="null" means not specifying vm option.
>>>>>>
>>>>>>>
>>>>>>> This is for:
>>>>>>> TestG1ConcMarkStepDurationMillis.java (G1)
>>>>>>> TestObjectTenuringFlags.java (Parallel)
>>>>>>> TestInitialTenuringThreshold.java (Parallel)
>>>>>>> TestG1HeapRegionSize.java (G1)
>>>>>> Okay, I will add these tags at next webrev.
>>>>>>
>>>>>> Thanks,
>>>>>> Sangheon
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>  - Derek
>>>>>>>
>>>>>>> On 8/24/15 5:33 PM, sangheon.kim wrote:
>>>>>>>> Hi Kim,
>>>>>>>>
>>>>>>>> On 08/24/2015 02:16 PM, Kim Barrett wrote:
>>>>>>>>> On Aug 24, 2015, at 3:06 PM, sangheon.kim <sangheon.kim at oracle.com> wrote:
>>>>>>>>>> Hi Kim,
>>>>>>>>>>
>>>>>>>>>> Here's webrev.03 which includes your comment for MarkStackSize constraint function.
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.03
>>>>>>>>>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.03_to_02/
>>>>>>>>>>
>>>>>>>>>> And all your comments will be managed by https://bugs.openjdk.java.net/browse/JDK-8134348 .
>>>>>>>>> If the value of MarkStackSizeMax were changed later, there's nothing
>>>>>>>>> to verify MarkStackSize is still smaller.  [This is related to my
>>>>>>>>> earlier comment about constraints between options being tested twice
>>>>>>>>> and failures reported twice.]  Do we care in this case?
>>>>>>>> If your concern is something like
>>>>>>>> -XX:MarkStackSize=128 -XX:MarkStackSizeMax=100.
>>>>>>>> Yes, in this case the order is important as ranges and constraint functions are verified by its order.
>>>>>>>> MarkStackSizeMax will be verified first(its range) and MarkStackSize will be compared with verified
>>>>>>>> MarkStackSizeMax.
>>>>>>>>
>>>>>>>> And as I said your original concern is current limitation.
>>>>>>>> If we set CMSOldPLABMin and CMSOldPLABMax together with invalid values (e.g. CMSOldPLABMin=100, CMSOldPLABMax=50),
>>>>>>>> they will print out 2 failure messages.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Sangheon
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Other than that, looks good.
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
>
>