Hi everyone,
I just talked this through with Jesper.
I'm fine with the latest webrev. To size the young gen the code relies
on that an old GC has happened. Currently there is nothing that
guarantees this, so the implementation kind of relies on the application
to call System.gc() to get the desired effect. That works for the
current use case (using the management APIs to set the values) but may
not work well for someone how just sets the values on the command line.
An alternative would be to check Min/MaxHeapFreeRatio at the end of a
scavenge and trigger an old collection if the limits have been exceeded.
That way the code would be more self sustained. Jesper will file a
separate RFE for that.
Bengt
On 1/29/14 9:25 PM, Jesper Wilhelmsson wrote:
Hi Bengt,
Just a short clarification inline. Looking forward to your comments
later today.
Bengt Rutisson skrev 29/1/14 4:41 PM:
Hi Jesper,
On 1/28/14 11:09 PM, Jesper Wilhelmsson wrote:
Bengt,
Thanks for looking at the change.
Answers inline.
Bengt Rutisson skrev 28/1/14 2:02 PM:
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 }
The intention of this code is to constrain the young space if
someone is using
the heap free ratio flags. Since it is a bit weird to talk about a
"free
ratio" in the young space, we use the heap free ratios to determine
the size
of the old generation, and then we use NewRatio to scale the young
generation
accordingly.
The use of NewSize and MaxNewSize shouldn't affect this decision at
this
point. They are mainly used to set the initial sizes and limits for
the young
generation which will be respected as we use the MIN of the NewRatio
calculation and the young_gen->max_size().
I agree that it is hard to define "free" for the young gen. But this
looks kind
of strange to me. We guard the setting of max_young_size with both
MinHeapFreeRatio or MaxHeapFreeRatio but we don't use any of them in the
calculation.
In psScavenge.cpp the flags MinHeapFreeRatio and MaxHeapFreeRatio is
only used to determine *if* we should limit the young gen size.
The flags are used to determine the size of the old generation. Then
we scale the young generation based on the limited old size using the
NewRatio flag.
I will add the following comment before the new if-statement:
// Deciding a free ratio in the young generation is tricky, so if
// MinHeapFreeRatio or MaxHeapFreeRatio are in use (implicating
// that the old generation size may have been limited because of them) we
// should then limit our young generation size using NewRatio to have it
// follow the old generation size.
We use the max_young_size for two purposes: calculating the survivor
size and
calculating the eden size. Maybe we can split it up somehow to get
understandable logic. I'll think a bit more about this and come back
later
tonight with some comments.
invoke_no_policy() is a huge method and clearly it would benefit the
code to split it into smaller parts. I'm not sure this particular part
is the most urgent to refactor though, plus I didn't want to mix this
change with a lot of cleanup.
The logic of the sizing calculation is not really changed more than
the possible adjustment of the upper limit of the young size. Instead
of using the hard upper limit of the young generation, we use the
NewRatio-based size if it's smaller. The young gen has two parts that
needs to be resized, eden and survivors. Both use the same limited
young gen size for their size calculations in the same way as they
before used the young gen max size.
There is only one other use of young_gen->max_size() in this code.
This is an assert that verifies that the parts of the young gen
doesn't exceed the hard upper limit. I think this one should still
look at the real limit, and not the calculated one.
This code should however only be executed if using adaptive size
policy so I
will add that to the if-statement.
That won't be necessary. That whole section is guarded by
UseAdaptiveSizePolicy.
Indeed it is. I noticed the check in line 561 and thought "Hey, this
code also needs that check", but it seems the check should be removed
from line 561 instead. :-)
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.
I went back and forth on this one, but decided that I wanted to
express that
if using adaptive size policy, the default values of these flags
should be
different. I think it would work perfectly fine if using
FLAG_SET_ERGO instead
but I'm thinking that this is not really an ergonomic decision, but
rather due
to a different implementation.
OK. I am also undecided on what's best, so let's leave it as it is.
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()?
This code moved from check_vm_args_consistency() to apply_ergo()
since it is a
ergonomic decision to change the value of the flag. I don't think
this kind of
changes should be done while checking argument consistency.
verify_MinHeapFreeRatio() is called from check_vm_args_consistency().
I don't see why it is wrong to check this as argument consistency.
Clearly
MinHeapFreeRatio can only be a value between 0 and 99. In my opinion
that would
be best to check at startup.
apply_ergo() is also done during startup, but I see your point. I'll
move the check into verify_MinHeapFreeRatio().
Thanks,
/Jesper
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?
Yes, that's what I want to achieve.
OK. Good.
(I assume that you mean "can't write MaxHeapFreeRatioMoreCharacters".)
Right ;)
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.
Dmitry is working on the tests for this feature. I'll ask him to
include a few
tests for illegal values as well.
OK.
Did you have a look at the test/gc/arguments/TestHeapFreeRatio.java
test? Is
that relevant to verify your changes?
No, my changes are not tested by TestHeapFreeRatio. I wrote a few
lines about
why towards the end of my last mail.
Sorry. Missed that. I will go back and check that email.
Thanks,
Bengt
Thanks,
/Jesper
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
