RFR (S) JDK-8153578,Default NewRatio is ignored when UseConcMarkSweepGC is used as GC algorithm

[Jesper Wilhelmsson]

The change looks good to me but you should verify with Jon that we are not 
breaking any customer assumptions by doing this change.
/Jesper


Den 25/5/16 kl. 18:19, skrev Joseph Provino:
> This is a simple change from MIN2 to MAX2 suggested by Jesper.
> The code now matches the comment and seems to fix the problem.
>
> Webrev:  http://cr.openjdk.java.net/~jprovino/8153578/webrev.01
>
> thanks.
>
> joe
>
> On 4/20/2016 6:29 PM, Jon Masamitsu wrote:
>>
>>
>> On 04/20/2016 12:35 PM, Jesper Wilhelmsson wrote:
>>> Hi Joe,
>>>
>>> If I understand the bug description correctly the problem is that NewSize
>>> becomes too small. According to the bug the VM ignores the NewRatio setting.
>>>
>>> Your change removes the setting of MaxNewSize in the case where NewSize has
>>> the default value. It's not obvious to me how that is related to the bug.
>>>
>>> There is an if statement enclosing the code you are changing. It has a
>>> comment that I find interesting:
>>>
>>> 1755   // If either MaxNewSize or NewRatio is set on the command line,
>>> 1756   // assume the user is trying to set the size of the young gen.
>>> 1757   if (FLAG_IS_DEFAULT(MaxNewSize) && FLAG_IS_DEFAULT(NewRatio)) {
>>>
>>> The interesting part is that the comment says "MaxNewSize OR NewRatio" but
>>> the code says "MaxNewSize AND NewRatio". This could be a typo in the comment,
>>> or it could be related to your bug.
>>>
>>> I don't think the fix here is to ignore the calculated
>>> preferred_max_new_size, but rather to figure out why it has the wrong value.
>>> preferred_max_new_size is calculated a few lines up, and it is based on
>>> NewRatio. The number of threads seems to be involved as well. Should it be?
>>> Usually things based on the number of threads tend to be wrong...
>>
>> The intent of this code was to control the young pause times by limiting the
>> size of
>> the young gen.  The preferred size scaling with
>>
>> young_gen_per_worker * ParallelGCThreads
>>
>> was meant to take into account the fact you could have approximately the
>> same pauses with larger heaps as the number of GC workers increases.
>> I didn't add this code but I'm pretty sure it was added as a result of
>> customer interaction.
>>
>> Jon
>>
>>>
>>> 1748     MIN2(max_heap/(NewRatio+1), ScaleForWordSize(young_gen_per_worker *
>>> ParallelGCThreads));
>>>
>>> young_gen_per_worker is CMSYoungGenPerWorker which defaults to things like
>>> 16M or 64M. ParallelGCThreads is usually just a handful, 8 on my machine.
>>> Since we take the smallest number of this thread based thing and the NewRatio
>>> calculation, I would guess the threads will limit the MaxNewSize quite a lot.
>>> I wonder if this isn't the bug you are looking for. It would make more sense
>>> to me if it was MAX of the two instead of MIN. You should probably consult
>>> whoever wrote this code.
>>>
>>> /Jesper
>>>
>>>
>>> Den 20/4/16 kl. 20:08, skrev Joseph Provino:
>>>> Please review this tiny change.  It only affects ParNew.  Are there any
>>>> unintended consequences?
>>>>
>>>> Passes JPRT.
>>>>
>>>> CR: JDK-8153578 Default NewRatio is ignored when UseConcMarkSweepGC is used as
>>>> GC algorithm <https://bugs.openjdk.java.net/browse/JDK-8153578>
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jprovino/8153578/webrev.00
>>>>
>>>>
>>
>