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

[Jesper Wilhelmsson]

Thanks for the review Bengt!
/Jesper

Bengt Rutisson skrev 11/11/13 1:31 PM:
>
> 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
>>>>
>>
>