RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable
Bernd Eckenfels
bernd-2014 at eckenfels.net
Wed Jan 29 20:37:00 UTC 2014
Hello,
I wonder if there is a need to clamp MinHeapFreeRatio to 99, as that
can be similiar unlikely to be achieved (in all cases the Heap will not
grow more than Xmx). Would it be more important to check if MinHeap <
MaxHeap? (which also ensures it can not be 100).
Bernd
Am Wed, 29 Jan 2014 16:41:53 +0100
schrieb Bengt Rutisson <bengt.rutisson at oracle.com>:
>
> 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.
>
> 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.
>
> >
> > 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.
>
> >
> >> 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.
>
> >
> >> 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.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