RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable

[Bengt Rutisson]

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;

(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.)

We also don't seem to take MinHeapFreeRatio into account. Should we do that?

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.

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?

I also agree with Staffan that the methods is_within() and is_min() make 
it harder to read the code.

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