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

[Joe Provino]

ok, thanks.

On 04/20/2016 04:22 PM, Jesper Wilhelmsson wrote:
> Den 20/4/16 kl. 22:00, skrev Joe Provino:
>> Hi Jesper, I had a feeling what looked like a simple fix wouldn't be 
>> that
>> simple.  ;-)
>>
>> On 04/20/2016 03: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.
>> If the code inside that "if" is executed, then the bug occurs. So I 
>> think
>> changing to OR won't fix the problem.
>
> No, it's probably not that simple. And it may be unrelated to your 
> bug. But either the code or the comment is wrong here.
>
>>>
>>> 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...
>>>
>>> 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.
>> Yeah, I don't know the reasoning behind this.
>>> You should probably consult whoever wrote this code.
>> That sounds like a good idea.  I'm not sure who that person is.
>
> Ramki added this code in the fix for 6896099 - Integrate CMS heap ergo 
> with default heap sizing ergo
>
> If you just want to try it out, I would suggest to see what happens if 
> you change MIN to MAX in line 1748.
> /Jesper
>
>>
>> thanks.
>>
>> joe
>>
>>>
>>> /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
>>>>
>>>>
>>