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

[sangheon.kim]

Forwarding to public alias.


>>
>> On 08/26/2015 05:37 PM, sangheon.kim wrote:
>>> Hi Jon,
>>>
>>>
>>> On 08/26/2015 03:18 PM, sangheon.kim wrote:
>>>> Hi Jon,
>>>>
>>>> Thank you for the review.
>>>>
>>>> On 08/26/2015 01:57 PM, Jon Masamitsu wrote:
>>>>> http://cr.openjdk.java.net/~sangheki/8078555/webrev.03/src/share/vm/runtime/commandLineFlagConstraintsGC.cpp.frames.html 
>>>>>
>>>>>
>>>>> Seems to me that a user should be able to set the default value for
>>>>> a flag on the command line without getting an error. Below if I set
>>>>> G1RSetRegionEntries to 0, I get a constraint violation.
>>>> I agree with you.
>>>> Let me change both constraint functions(G1RSetRegionEntries, 
>>>> G1RSetSparseRegionEntries) and related 
>>>> codes(HeapRegionRemSet::setup_remset_size).
>>> Looking at the some flags, our codes are working differently for 
>>> default value of zero vs. manually set value of zero.
>>> We are just calling FLAG_IS_DEFAULT(xxx) to check whether it is 
>>> set(default value) or not.
>>> So it seems like xxx=0 is not recognized as default value even 
>>> though xxx's default value is zero.
>>> What do you think?
>>> Please look at MaxGCPauseMillis, GCPauseIntervalMillis, 
>>> G1RSetSparseRegionEntries and G1RSetRegionEntries.
> You are right.  We should not let the user set the flag to the default
> value in the cases of G1RSetRegionEntries and G1RSetSparseRegionEntries.
>
> For MaxGCPauseMillis and GCPauseIntervalMillis the users should not be 
> allowed to set the
> values to 0.  That is already checked in the G1ColllectedHeap 
> constructor.  I don't think you
> need to make any added changes for those.
>
> Does that answer your questions?
Yes, thank you for the answer.

So I will not change for your comment of 'setting 0 for 
G1RSetRegionEntries'.

Thanks,
Sangheon


>
> Jon
>>>
>>> Thanks,
>>> Sangheon
>>>
>>>
>>>>
>>>> FYI, if we don't stop with zero by constraint function, it fires 
>>>> assert under current code.
>>>>
>>>> These will be included at next webrev.
>>>>
>>>> Thanks,
>>>> Sangheon
>>>>
>>>>>
>>>>> 271 Flag::Error G1RSetRegionEntriesConstraintFunc(intx value, bool 
>>>>> verbose) {
>>>>> 272 if (!UseG1GC) return Flag::SUCCESS;
>>>>> 273
>>>>> 274 // Default value of G1RSetRegionEntries=0 means will be set 
>>>>> ergonomically.
>>>>> 275 // Minimum value is 1.
>>>>> 276 if (FLAG_IS_CMDLINE(G1RSetRegionEntries) && (value < 1)) {
>>>>> 277 CommandLineError::print(verbose,
>>>>> 278 "G1RSetRegionEntries (" INTX_FORMAT ") must be "
>>>>> 279 "greater than or equal to 1\n",
>>>>> 280 value);
>>>>> 281 return Flag::VIOLATES_CONSTRAINT;
>>>>> 282 } else {
>>>>> 283 return Flag::SUCCESS;
>>>>> 284 }
>>>>> 285 }
>>>>>
>>>>>
>>>>>
>>>>> 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.
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>