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

[Jesper Wilhelmsson]

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