RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable
Bengt Rutisson
bengt.rutisson at oracle.com
Wed Jan 29 07:41:53 PST 2014
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