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
