RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable

[Jesper Wilhelmsson]

Thanks for the review!
/Jesper

Staffan Larsen skrev 28/1/14 8:09 AM:
> Looks good from my point of view.
>
> /Staffan
>
> On 27 jan 2014, at 21:46, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com
> <mailto: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
>>>> <mailto: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
>