RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable

[Jesper Wilhelmsson]

Thanks for the review Mikael!
/Jesper

Mikael Gerdin skrev 29/1/14 3:48 PM:
> Hi Jesper,
>
> On Monday 27 January 2014 21.46.01 Jesper Wilhelmsson wrote:
>> Staffan, Bengt, Mikael,
>>
>> Thanks for the reviews!
>>
>> I have made the changes you have suggested and a new webrev is available at:
>>
>> http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.5/
>>
>> I agree with your assessment that it would be good to implement a generic
>> way to verify manageable flags. I think it is a separate change though so I
>> will not attack that problem in this change.
>>
>> As Mikael wrote in his review we have talked offline about the changes and
>> how to make them more correct and readable. Thanks Mikael for the input!
>
> The calculations are much easier to follow now, thanks.
>
> I think the changes are good.
>
> /Mikael
>
>>
>> More comments inline.
>>
>> Bengt Rutisson skrev 22/1/14 11:21 AM:
>>> Hi Jesper,
>>>
>>> The calculation in
>>> PSAdaptiveSizePolicy::calculated_old_free_size_in_bytes()>
>>> looks wrong to me. I would have expected this:
>>>     86     // free = (live*ratio) / (1-ratio)
>>>     87     size_t max_free = (size_t)((heap->old_gen()->used_in_bytes() *
>>>
>>> mhfr_as_percent) / (1.0 - mhfr_as_percent));
>>>
>>> to be something like this:
>>>    size_t max_free = heap->old_gen()->capacity_in_bytes() *
>>>    mhfr_as_percent;
>>
>> The suggested formula above will calculate how much free memory there can be
>> based on the current old gen size. What I want to achieve in the code is to
>> calculate how much free memory there can be based on the amount of live
>> data in the old generation. I have talked to Bengt offline and he agrees
>> that the code is doing what I want it to. I have rewritten the code and
>> added more comments to explain the formula.
>>
>>> (A minor naming thing is that mhfr_as_percent is actually not a percent
>>> but a ratio or fraction. Just like you write in the comment.)
>>
>> Right. Fixed.
>>
>>> We also don't seem to take MinHeapFreeRatio into account. Should we do
>>> that?
>> We should. Good catch! I have added support for MinHeapFreeRatio both here
>> and in psScavenge.cpp.
>>
>>> I think it should be possible to write a internal VM test or a whitebox
>>> test for the calculated_old_free_size_in_bytes() to verify that it
>>> produces the correct results.
>>
>> I've added an internal test to verify the new code.
>>
>>> Speaking of testing. There is already a test called
>>> test/gc/arguments/TestHeapFreeRatio.java. That test seems to pass with the
>>> ParallelGC already before your changes. I think that means that the test
>>> is not strict enough. Could you update that test or add a new test to
>>> make sure that your changes are tested?
>>
>> TestHeapFreeRatio only verifies that the VM gives correct error messages for
>> the -Xminf and -Xmaxf flags. Since HotSpot usually don't complain about
>> flags that don't affect the chosen GC, there is no error given about
>> ParallelGC not implementing the heap ratio flags. The code I change is not
>> tested by this test. Dmitry Fazunenko has developed a test for the new
>> feature which I have used while developing. This test will be pushed once
>> the feature is in place.
>>> I also agree with Staffan that the methods is_within() and is_min() make
>>> it
>>> harder to read the code.
>>
>> Yes, me to...
>> I removed them.
>>
>> Thanks,
>> /Jesper
>>
>>> Thanks,
>>> Bengt
>>>
>>> On 2014-01-22 09:40, Staffan Larsen wrote:
>>>> Jesper,
>>>>
>>>> This looks ok from a serviceability perspective. Long term we should
>>>> probably have a more pluggable way to verify values of manageable flags
>>>> so we can avoid some of the duplication.
>>>>
>>>> I have a slight problem with is_within() and is_min() in that it is not
>>>> obvious from the call site if the min and max values are inclusive or not
>>>> - it was very obvious before.
>>>>
>>>> /Staffan
>>>>
>>>>
>>>> On 21 jan 2014, at 22:49, Jesper Wilhelmsson
>>>> <jesper.wilhelmsson at oracle.com>>>
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>> Could I have a few reviews of this change?
>>>>>
>>>>> Summary:
>>>>> To allow applications a more fine grained control over the GC over time,
>>>>> we'll make the flags MinHeapFreeRatio and MaxHeapFreeRatio manageable.
>>>>>
>>>>> The initial request that lead up to this change involved ParallelGC
>>>>> which is notoriously unwilling to shrink the heap. Since ParallelGC
>>>>> didn't support the heap free ratio flags, this change also includes
>>>>> implementing support for these flags in ParallelGC.
>>>>>
>>>>> Changes have also been made to the argument parsing, attach listener and
>>>>> the management API to verify the flag values when set through the
>>>>> different interfaces.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.4/
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8028391
>>>>>
>>>>> The plan is to push this to 9 and then backport to 8 and 7.
>>>>>
>>>>> Thanks!
>>>>> /Jesper
>