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

[Stefan Johansson]

On 2016-05-19 17:49, Jon Masamitsu wrote:
>
>
> 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 for the review Jon.

I'll push this on Monday and I will add this comment to _adaptive_size 
if no one objects:
+  // False when using a fixed young generation size due to command-line 
options,
+  // true otherwise.
    bool _adaptive_size;

Thanks,
Stefan

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