RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable (original) (raw)

Jesper Wilhelmsson jesper.wilhelmsson at oracle.com
Tue Jan 28 22:10:07 UTC 2014

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::calculatedoldfreesizeinbytes() looks wrong to me. I would have expected this: 86 // free = (live*ratio) / (1-ratio) 87 sizet maxfree = (sizet)((heap->oldgen()->usedinbytes() * mhfraspercent) / (1.0 - mhfraspercent)); to be something like this: sizet maxfree = heap->oldgen()->capacityinbytes() * mhfraspercent; 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 mhfraspercent 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 calculatedoldfreesizeinbytes() 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 iswithin() and ismin() 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 iswithin() and ismin() 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

More information about the hotspot-gc-dev mailing list