RFR(S): JDK-8028093 - Initial young size is smaller than minimum young size

[Bengt Rutisson]

Hi all,

I talked this through with Jesper.

He convinced me that the behavior after this change is the same as 
before JDK-8016309. But there are probably bugs lurking in this code. It 
is very strange that we mix min and initial values in the adjust methods.

However, I'm fine with pushing this change since it reverts to the old 
behavior.

Bengt


On 2013-11-11 12:56, Bengt Rutisson wrote:
>
> Jepser,
>
> On 2013-11-11 12:03, Jesper Wilhelmsson wrote:
>> Bengt,
>>
>> The intention of the min_gen1_size argument is to make sure there is 
>> space left in the heap for the old generation. The minimum size of 
>> the old generation is _min_gen1_size in both cases. In the current 
>> code you can see that the argument I want to remove is called 
>> min_gen1_size and the code expects to be called with _min_gen1_size 
>> at all times. This was also the case before the fix for JDK-8016309, 
>> so this bug was actually introduced by the collector policy cleanup.
>
> It does not look like the intent of adjust_gen0_sizes() is to always 
> use min_gen1_size. It just has badly named parameters.
>
> To me, adjust_gen0_sizes() looks like it takes three values, 
> gen0_size, gen1_size and heap_size, and then adjusts them if they 
> don't match. It's odd that it would also adjust for the min_gen1_size 
> in all cases. That should be done separately.
>
> Bengt
>
>>
>> Thanks,
>> /Jesper
>>
>> Bengt Rutisson skrev 11/11/13 10:32 AM:
>>>
>>> Hi Jesper,
>>>
>>> This looks wrong to me.
>>>
>>> adjust_gen0_sizes() is first called with the min values to adjust 
>>> them. Then
>>> called with the initial values to adjust them. With your suggested 
>>> change we
>>> suddenly mix min and initial values when we adjust. That seems very 
>>> strange to me.
>>>
>>> If the issue is that the min and initial values don't match I think 
>>> we should
>>> add a third step where we explicitly adjust them to match. Something 
>>> like:
>>>
>>> adjust_gen0_sizes(&_min_gen0_size, &_min_gen1_size, 
>>> _min_heap_byte_size,
>>> _min_gen1_size);
>>> adjust_gen0_sizes(&_initial_gen0_size, &_initial_gen1_size,
>>> _initial_heap_byte_size, _initial_gen1_size);
>>> adjust_min_and_initial_values(...);
>>>
>>> Thanks,
>>> Bengt
>>>
>>> On 2013-11-10 00:17, Jesper Wilhelmsson wrote:
>>>> Hi,
>>>>
>>>> Can I have a couple of reviews of this small bugfix.
>>>>
>>>> Bug JDK-8028093: https://bugs.openjdk.java.net/browse/JDK-8028093
>>>>
>>>> The problem is that 
>>>> TwoGenerationCollectorPolicy::adjust_gen0_sizes() is
>>>> called with the wrong argument in one place. The last argument 
>>>> should always
>>>> be the value of _min_gen1_size, but in one call it is the 
>>>> _initial_gen1_size.
>>>>
>>>> Since these variables are class local I see no reason to take 
>>>> _min_gen1_size
>>>> as an argument at all. Apparently it is error prone. The proposed 
>>>> fix is
>>>> therefore to remove the argument and use _min_gen1_size directly in
>>>> adjust_gen0_sizes().
>>>>
>>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8028093/webrev/
>>>>
>>>> Thanks!
>>>> /Jesper
>>>
>