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.wilhelms...@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
