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

[Jon Masamitsu]

Joe,

The intention of this code is to ergonomically select a small young gen
size to keep young GC pauses small.  The value of default value of
CMSYoungGenPerWorker was chosen such that 1 GC worker could
collect that large a young gen in an "acceptable" pause time.
"acceptable", of course, is in the eye of the beholder and is
probably out of date but it allows the user to pick a value  such
that the young gen gets larger as the ability to collect the young
gen in that "acceptable" pause time scales up with the number
of GC workers.

Bottom line is that this is not a bug.  It is an unfortunate conflict
between the default behavior of how the other GC's use NewRatio
and how CMS uses NewRatio.  I think there is a similiar conflict
in G1.

I'll add this to the CR.

Jon


On 5/25/2016 9:19 AM, Joseph Provino wrote:
> 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
>>>>
>>>>
>>
>