[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8028391 - Make the Min/MaxHeapFreeRatio flags manageable
From:       Jesper Wilhelmsson <jesper.wilhelmsson () oracle ! com>
Date:       2014-01-29 21:35:51
Message-ID: 52E97437.5080407 () oracle ! com
[Download RAW message or body]

Thanks for the review Bengt!
The RFE is filed here: https://bugs.openjdk.java.net/browse/JDK-8033206
/Jesper

Bengt Rutisson skrev 29/1/14 10:09 PM:
>
> 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.wilhelmsson@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
>>>>>>>
>>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]