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

[Jon Masamitsu]

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.

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