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

[sangheon]

Hi,

On 05/20/2016 09:52 AM, Jon Masamitsu wrote:
>
>
> On 5/20/2016 5:20 AM, Stefan Johansson wrote:
>>
>>
>> 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.
>
> Unless the heap size is fixed (max heap size == min heap size), I 
> would not
> describe the NewRatio case as "fixed" because the NewRatio case sets
> the max eden  and min eden sizes but eden can still vary within those
> limits.   Am I wrong?
Jon, as Stefan said g1YoungGenSizer.hpp line 59 explains about 'fixed'.
And looking at the code, if we only set NewRatio we reach '_sizer_kind = 
SizerNewRatio' at line 36 g1YoungGenSizer.cpp and 
G1YoungGenSizer::recalculate_min_max_young_length() sets min/max size to 
be same for SizerNewRatio case, so it means 'fixed'.

However, I think it is more appropriate to make 'NewRatio only set case' 
to 'not-fixed' as you said.

Stefan, change looks good to me as well.

Thanks,
Sangheon


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