Looks good from my point of view. /Staffan
On 27 jan 2014, at 21:46, Jesper Wilhelmsson <[email protected]> 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 <[email protected]> >>> 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
