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

[Jon Masamitsu]

On 08/27/2015 12:44 PM, sangheon.kim wrote:
>
>
> 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.

Yes.

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

Ok.

Reviewed.

Jon

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