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

[sangheon]

Hi Stefan,

On 05/23/2016 01:20 AM, Stefan Johansson wrote:
>
>
> On 2016-05-21 00:47, sangheon wrote:
>> 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.
>>
> I agree that this might make sense, but it would be a big change to 
> how G1 works for this option and I don't think this is something we 
> should change this late in the release. If you like to investigate 
> this further please file an RFE.
I totally agree with your opinion.
I think it is okay to start the investigation about changing it if we need.

Thanks,
Sangheon

>
>> Stefan, change looks good to me as well.
>
> Thanks,
> Stefan
>>
>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>