RFR: 8156028: G1YoungGenSizer _adaptive_size not correct when setting NewSize and MaxNewSize to the same value

[Jon Masamitsu]

On 05/19/2016 02:27 AM, Stefan Johansson wrote:
>
>
> On 2016-05-18 20:51, Jon Masamitsu wrote:
>> Stefan,
>>
>> I think your change make the code a little more correct
>> but it still confuses me some.  The code that sets _adaptive_size
>> to false here
>>
>>>    30 G1YoungGenSizer::G1YoungGenSizer() : 
>>> _sizer_kind(SizerDefaults), _adaptive_size(true),
>>>    31         _min_desired_young_length(0), 
>>> _max_desired_young_length(0) {
>>>    32   if (FLAG_IS_CMDLINE(NewRatio)) {
>>>    33     if (FLAG_IS_CMDLINE(NewSize) || 
>>> FLAG_IS_CMDLINE(MaxNewSize)) {
>>>    34       log_warning(gc, ergo)("-XX:NewSize and -XX:MaxNewSize 
>>> override -XX:NewRatio");
>>>    35     } else {
>>>    36       _sizer_kind = SizerNewRatio;
>>>    37       _adaptive_size = false;
>>>    38       return;
>>>    39     }
>>>    40   }
>>
>> says to me that _adaptive_size == true means that G1 is
>> being allowed to set the min and max size as it needs to.
>> It doesn't say that the min and max are the same (since
>> the size is allowed to vary within the bounds set by
>> NewRatio and the min and max of the entire heap).
>>
>> Does that make sense?
>>
>
> Not really, _adaptive_size == true means that the young generation 
> size can vary between the min and max values specified. This is 
> explained in g1YoungGenSizer.hpp. There are two sentences about 
> _adaptive_size in there, but they described this as setting a fixed size:
> 1. There is a special case when NewSize==MaxNewSize. This is 
> interpreted as "fixed"
> 2. If only -XX:NewRatio is set we should use the specified ratio of 
> the heap as both min and max. This will be interpreted as "fixed" just 
> like the NewSize==MaxNewSize case above.
>
> Point 2 is already correct in the code you pasted above, 
> _adaptive_size is set to false when only NewRation is set. Point 1 is 
> what I'm fixing in this change.
>
> Do you agree?

I agree that you are fixing 1. so the change is good.

I still find the concept behind _adaptive_size unclear.
Could you add a specification for  _adaptive_size in the
header file?  I don't need another webrev if you decide
to add the comment.

Thanks.

Jon
>
> Thanks,
> Stefan
>
>> Jon
>>
>>
>> On 05/18/2016 07:48 AM, Stefan Johansson wrote:
>>>
>>> Please review this small change for:
>>> https://bugs.openjdk.java.net/browse/JDK-8156028
>>>
>>> Webrev:
>>> http://cr.openjdk.java.net/~sjohanss/8156028/hotspot.00/
>>>
>>> Summary:
>>> The comparison in G1YoungGenSizer is wrong, when NewSize and 
>>> MaxNewSize is equal the sizer should not be adaptive.
>>>
>>> Testing:
>>> * JPRT
>>> * RBT
>>>
>>> Thanks,
>>> Stefan
>>
>