RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable
Jesper Wilhelmsson
jesper.wilhelmsson at oracle.com
Wed Jan 29 13:34:22 PST 2014
Hi Bernd,
This change did not introduce setting the MinHeapFreeRatio to 99, I just moved
it to a different place in the code.
Currently it is allowed to set Min = Max. I can't say I have given this enough
thought to determine if that is good or bad. Anyhow, changing that would affect
all collectors so it would have to be done in a separate change.
Cheers,
/Jesper
Bernd Eckenfels skrev 29/1/14 9:37 PM:
> 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 serviceability-dev
mailing list