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

[Stefan Johansson]

On 2016-05-23 17:50, Jon Masamitsu wrote:
>
>
> On 05/23/2016 01:09 AM, Stefan Johansson wrote:
>>
>>
>> On 2016-05-20 18:52, 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?
>>>
>> For G1 at least. If NewRatio is specified, G1YoungGenSizer will use 
>> this code when changing the heap size to recalculate the young 
>> generation size:
>> src/share/vm/gc/g1/g1YoungGenSizer.cpp
>>  100     case SizerNewRatio:
>>  101       *min_young_length = number_of_heap_regions / (NewRatio + 1);
>>  102       *max_young_length = *min_young_length;
>>  103       break;
>>  104     default:
>>
>> Here number_of_heap_regions is the current number of regions used by 
>> the heap, and this code gets called whenever the heap size is 
>> changed. So in some sense the young generation size will be "fixed" 
>> until the next heap re-sizing. I agree that "fixed" is not a perfect 
>> term here but I used it because the class description used it.
>>
>> Would you like this comment better:
>> // True when the young generation bounds differ and allows a varying 
>> size,
>> // false otherwise.
>
> Is this correct and do you like it?
>
> // False if the young generation size bounds are the result of command 
> line options.
> // True otherwise.
>
Not entirely correct, if MinNewSize and MaxNewSize are set but to 
different values _adaptive_size will be True, since the size can vary 
between the two set values.

I'll go with my first comment.

Thanks for reviewing this Jon,
Stefan

> If you don't like that, then use your first comment.
>
> +  // False when using a fixed young generation size due to 
> command-line options,
> +  // true otherwise.
>
> Jon
>>
>> Stefan
>>
>>> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>