RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable

[Staffan Larsen]

Looks good from my point of view.

/Staffan

On 27 jan 2014, at 21:46, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> 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!
> 
> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20140128/ea594fed/attachment.htm>