RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable
Bengt Rutisson
bengt.rutisson at oracle.com
Tue Jan 28 05:02:01 PST 2014
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.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