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

[sangheon.kim]

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.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>