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

[sangheon.kim]

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

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