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

[Jon Masamitsu]

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

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