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

[Stefan Johansson]

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.

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