RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable

[Mikael Gerdin]

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