Hi Jesper,
On 2014-01-27 21:46, 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/
Can you explain this code in psScavenge.cpp a bit? I am not sure I
understand what it wants to achieve and how it works if I have set
NewSize and/or MaxNewSize on the command line.
532 size_t max_young_size = young_gen->max_size();
533 if (MinHeapFreeRatio != 0 || MaxHeapFreeRatio != 100) {
534 max_young_size = MIN2(old_gen->capacity_in_bytes() /
NewRatio, young_gen->max_size());
535 }
In arguments.cpp:
1572 if (UseAdaptiveSizePolicy) {
1573 // We don't want to limit adaptive heap sizing's freedom to
adjust the heap
1574 // unless the user actually sets these flags.
1575 if (FLAG_IS_DEFAULT(MinHeapFreeRatio)) {
1576 FLAG_SET_DEFAULT(MinHeapFreeRatio, 0);
1577 }
1578 if (FLAG_IS_DEFAULT(MaxHeapFreeRatio)) {
1579 FLAG_SET_DEFAULT(MaxHeapFreeRatio, 100);
1580 }
1581 }
Should these be FLAG_SET_ERGO instead? Not sure. Just asking.
3705 if (MinHeapFreeRatio == 100) {
3706 // Keeping the heap 100% free is hard ;-) so limit it to 99%.
3707 FLAG_SET_ERGO(uintx, MinHeapFreeRatio, 99);
3708 }
Couldn't this just be part of Arguments::verify_MinHeapFreeRatio()?
attachListener.cpp
strncmp(name, "MaxHeapFreeRatio", 17) == 0
MaxHeapFreeRatio is 16 characters. Is the 17th character in the constant
always NULL and this check verifies that I can write
MaxHeapFreeRatioMoreCharacters and get it to pass the strncmp?
It would be nice with a JTreg test that sets the flags to valid and
invalid values and checks that the flags have the right values after this.
Did you have a look at the test/gc/arguments/TestHeapFreeRatio.java
test? Is that relevant to verify your changes?
Thanks,
Bengt
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
<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
