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

[sangheon.kim]

On 08/27/2015 11:17 AM, Jon Masamitsu wrote:
> http://cr.openjdk.java.net/~sangheki/8078555/webrev.03/src/share/vm/runtime/commandLineFlagConstraintsGC.cpp.frames.html 
>
>
> I'd suggest adding more explanation about why the constraint failed.
>
>  190 static Flag::Error 
> CheckMaxHeapSizeAndSoftRefLRUPolicyMSPerMB(size_t maxHeap, intx 
> softRef, bool verbose) {
>  191   if ((softRef > 0) && ((maxHeap / M) > (max_uintx / softRef))) {
>  192     CommandLineError::print(verbose,
>
>         "Desired life time of SoftReferences cannot be expressed 
> correctly. "
>
>
>  193                             "MaxHeapSize (" SIZE_FORMAT ") or 
> SoftRefLRUPolicyMSPerMB "
>  194                             "(" INTX_FORMAT ") is too large\n",
>  195                             maxHeap, softRef);
>  196     return Flag::VIOLATES_CONSTRAINT;
>  197   } else {
>
Your suggestion is adding "Desired ... correctly.", right?
Okay, I will add it.

>
> Should there be a call to MinPLABSizeBounds() in this  function? There
> are checks that the values is not greater than the maximum but
> not a check that the value is less than the minimum.
>
>  374 Flag::Error CMSOldPLABMinConstraintFunc(size_t value, bool 
> verbose) {
>  375   Flag::Error status = Flag::SUCCESS;
>  376
>  377 #if INCLUDE_ALL_GCS
>  378   if (UseConcMarkSweepGC) {
>  379     if (value > CMSOldPLABMax) {
>  380       CommandLineError::print(verbose,
>  381                               "CMSOldPLABMin (" SIZE_FORMAT ") 
> must be "
>  382                               "less than or equal to 
> CMSOldPLABMax (" SIZE_FORMAT ")\n",
>  383                               value, CMSOldPLABMax);
>  384       return Flag::VIOLATES_CONSTRAINT;
>  385     }
>  386     status = MaxPLABSizeBounds("CMSOldPLABMin", value, verbose);
>  387   }
>  388 #endif
>  389   return status;
>  390 }
Didn't added same reason of OldPLABSizeConstraintFunc().
As OldPLABSize for CMS has different meaning, it cannot compared with 
MinPLABSizeBounds().
(from globals.hpp: Minimum size of CMS gen promotion LAB caches per 
worker per block size.)

FYI, default CMSOldPLABMin is 16 while PLAB::min_size() on my linux 
machine returns 256.

Thanks,
Sangheon


>
> That's all.
>
> Jon
>
>
> On 08/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.
>>>>>
>>>>
>>>
>>
>