<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">Looks good from my point of view.<div><br></div><div>/Staffan</div><div><br><div><div>On 27 jan 2014, at 21:46, Jesper Wilhelmsson <<a href="mailto:jesper.wilhelmsson@oracle.com">jesper.wilhelmsson@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 16px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Staffan, Bengt, Mikael,<br><br>Thanks for the reviews!<br><br>I have made the changes you have suggested and a new webrev is available at:<br><br><a href="http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.5/">http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.5/</a><br><br>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.<br><br>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!<br><br>More comments inline.<br><br>Bengt Rutisson skrev 22/1/14 11:21 AM:<br><blockquote type="cite"><br>Hi Jesper,<br><br>The calculation in PSAdaptiveSizePolicy::calculated_old_free_size_in_bytes()<br>looks wrong to me. I would have expected this:<br><br> 86 // free = (live*ratio) / (1-ratio)<br> 87 size_t max_free = (size_t)((heap->old_gen()->used_in_bytes() *<br>mhfr_as_percent) / (1.0 - mhfr_as_percent));<br><br>to be something like this:<br><br> size_t max_free = heap->old_gen()->capacity_in_bytes() * mhfr_as_percent;<br></blockquote><br>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.<br><br><blockquote type="cite">(A minor naming thing is that mhfr_as_percent is actually not a percent but a<br>ratio or fraction. Just like you write in the comment.)<br></blockquote><br>Right. Fixed.<br><br><blockquote type="cite">We also don't seem to take MinHeapFreeRatio into account. Should we do that?<br></blockquote><br>We should. Good catch! I have added support for MinHeapFreeRatio both here and in psScavenge.cpp.<br><br><blockquote type="cite">I think it should be possible to write a internal VM test or a whitebox test for<br>the calculated_old_free_size_in_bytes() to verify that it produces the correct<br>results.<br></blockquote><br>I've added an internal test to verify the new code.<br><br><blockquote type="cite">Speaking of testing. There is already a test called<br>test/gc/arguments/TestHeapFreeRatio.java. That test seems to pass with the<br>ParallelGC already before your changes. I think that means that the test is not<br>strict enough. Could you update that test or add a new test to make sure that<br>your changes are tested?<br></blockquote><br>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.<br><br><blockquote type="cite">I also agree with Staffan that the methods is_within() and is_min() make it<br>harder to read the code.<br></blockquote><br>Yes, me to...<br>I removed them.<br><br>Thanks,<br>/Jesper<br><br><br><blockquote type="cite"><br>Thanks,<br>Bengt<br><br><br><br>On 2014-01-22 09:40, Staffan Larsen wrote:<br><blockquote type="cite">Jesper,<br><br>This looks ok from a serviceability perspective. Long term we should probably<br>have a more pluggable way to verify values of manageable flags so we can avoid<br>some of the duplication.<br><br>I have a slight problem with is_within() and is_min() in that it is not<br>obvious from the call site if the min and max values are inclusive or not - it<br>was very obvious before.<br><br>/Staffan<br><br><br>On 21 jan 2014, at 22:49, Jesper Wilhelmsson <<a href="mailto:jesper.wilhelmsson@oracle.com">jesper.wilhelmsson@oracle.com</a>><br>wrote:<br><br><blockquote type="cite">Hi,<br><br>Could I have a few reviews of this change?<br><br>Summary:<br>To allow applications a more fine grained control over the GC over time,<br>we'll make the flags MinHeapFreeRatio and MaxHeapFreeRatio manageable.<br><br>The initial request that lead up to this change involved ParallelGC which is<br>notoriously unwilling to shrink the heap. Since ParallelGC didn't support the<br>heap free ratio flags, this change also includes implementing support for<br>these flags in ParallelGC.<br><br>Changes have also been made to the argument parsing, attach listener and the<br>management API to verify the flag values when set through the different<br>interfaces.<br><br>Webrev: <a href="http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.4/">http://cr.openjdk.java.net/~jwilhelm/8028391/webrev.4/</a><br><br>Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8028391">https://bugs.openjdk.java.net/browse/JDK-8028391</a><br><br>The plan is to push this to 9 and then backport to 8 and 7.<br><br>Thanks!<br>/Jesper</blockquote></blockquote></blockquote></div></blockquote></div><br></div></body></html>