[9] RFR (S) 8146436: Add -XX:+UseAggressiveHeapShrink option
Jon Masamitsu
jon.masamitsu at oracle.com
Tue Mar 1 23:05:57 UTC 2016
On 2/29/2016 10:10 PM, Chris Plummer wrote:
> On 2/25/16 10:23 AM, Chris Plummer wrote:
>> On 2/22/16 8:19 AM, Chris Plummer wrote:
>>> On 2/18/16 7:21 AM, Tom Benson wrote:
>>>> Hi Chris,
>>>>
>>>> On 2/10/2016 3:53 PM, Chris Plummer wrote:
>>>>> On 2/10/16 12:50 PM, Tom Benson wrote:
>>>>>>
>>>>>> I've heard from another GC team person that there might be more
>>>>>> feedback on the name coming, after some discussion. Not sure if
>>>>>> it will constitute the 'landslide' I mentioned. 8^)
>>>>> No problem. I'll wait for that to settle before sending out a
>>>>> final webrev.
>>>>
>>>> "Landslide" may not be the right term, but there was mild consensus
>>>> among the GC team that it's worth going through CCC again to
>>>> replace UseAggressiveHeapShrink. Our suggestion is
>>>> "ShrinkHeapInSteps", which would be on by default, and hopefully
>>>> describes what's happening without any other implications. So
>>>> you'd disable it to get what you want.
>>>> Tom
>>> Hi Tom,
>>>
>>> Ok, I'll get started with this. Just got back from vacation today,
>>> but it shouldn't take long to get to it.
>>>
>> Updated webrev:
>>
>> http://cr.openjdk.java.net/~cjplummer/8146436/webrev.03/
>>
>> thanks,
> Ping. Tom and Jon can you give final approval for this?
Changes look good. Reviewed.
Jon
>
> Thanks.
>
> Chris
>>
>> Chris
>>
>>> thanks,
>>>
>>> Chris
>>>>
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>> Tom
>>>>>>
>>>>>> On 2/10/2016 3:39 PM, Chris Plummer wrote:
>>>>>>> Hi Tom,
>>>>>>>
>>>>>>> if (!UseAggressiveHeapShrink) {
>>>>>>> // If UseAggressiveHeapShrink is false (the default),
>>>>>>> // we don't want shrink all the way back to initSize if
>>>>>>> people call
>>>>>>> // System.gc(), because some programs do that between
>>>>>>> "phases" and then
>>>>>>> // we'd just have to grow the heap up again for the next
>>>>>>> phase. So we
>>>>>>> // damp the shrinking: 0% on the first call, 10% on the
>>>>>>> second call, 40%
>>>>>>> // on the third call, and 100% by the fourth call. But
>>>>>>> if we recompute
>>>>>>> // size without shrinking, it goes back to 0%.
>>>>>>> shrink_bytes = shrink_bytes / 100 * current_shrink_factor;
>>>>>>> }
>>>>>>> assert(shrink_bytes <= max_shrink_bytes, "invalid shrink
>>>>>>> size");
>>>>>>> if (current_shrink_factor == 0) {
>>>>>>> _shrink_factor = 10;
>>>>>>> } else {
>>>>>>> _shrink_factor = MIN2(current_shrink_factor * 4,
>>>>>>> (size_t) 100);
>>>>>>> }
>>>>>>>
>>>>>>> I got rid of the changes at the start of the method, and added
>>>>>>> the !UseAggressiveHeapShrink check and the comment, so the first
>>>>>>> 2 lines above and the closing right brace are now the only
>>>>>>> change in the file, other than the copyright date. If you want I
>>>>>>> could also move the _shrink_factor adjustment into this block
>>>>>>> since the value of _shrink_factor becomes irrelevant if
>>>>>>> UseAggressiveHeapShrink is true. The assert should remain
>>>>>>> outside the block.
>>>>>>>
>>>>>>> cheers,
>>>>>>>
>>>>>>> Chris
>>>>>>>
>>>>>>> On 2/10/16 12:16 PM, Tom Benson wrote:
>>>>>>>> Hi Chris,
>>>>>>>> OK, that all sounds good.
>>>>>>>>
>>>>>>>> >> I can change it, although that will mean filing a new CCC.
>>>>>>>> Ah, I'd forgotten about that. Not worth it, unless there's a
>>>>>>>> landslide of support for a different name.
>>>>>>>>
>>>>>>>> Tnx,
>>>>>>>> Tom
>>>>>>>>
>>>>>>>> On 2/10/2016 3:06 PM, Chris Plummer wrote:
>>>>>>>>> Hi Tom,
>>>>>>>>>
>>>>>>>>> Thanks for having a look. Comments inline below:
>>>>>>>>>
>>>>>>>>> On 2/10/16 11:27 AM, Tom Benson wrote:
>>>>>>>>>> Hi Chris,
>>>>>>>>>> My apologies if I missed the discussion somewhere, but is
>>>>>>>>>> there a specific rationale for adding this that can be
>>>>>>>>>> mentioned in the bug report? I can imagine scenarios where
>>>>>>>>>> it would be useful, but maybe the real need can be called out.
>>>>>>>>> In general, it is for customers that want to minimize the
>>>>>>>>> amount of memory used by the java heap, and are willing to
>>>>>>>>> sacrifice some performance (induce more frequent GCs) to save
>>>>>>>>> that memory. When heap usage fluctuates greatly, the GC will
>>>>>>>>> tend to hold on to that memory longer than needed due to the
>>>>>>>>> the current algorithm which requires 4 full GCs before
>>>>>>>>> MaxHeapFreeRatio is fully honored. If this is what you are
>>>>>>>>> looking for, I can add it to the CR.
>>>>>>>>>>
>>>>>>>>>> I think it might be clearer if the new code in cardGeneration
>>>>>>>>>> was moved down to where the values are used. IE, I would
>>>>>>>>>> leave the inits of current_shrink_factor and _shrink_factor
>>>>>>>>>> as they were at lines 190/191. Then down at 270, just don't
>>>>>>>>>> divide by the shrink factor if UseAggressiveHeapShrink is
>>>>>>>>>> set, and the updates to shrink factor can be in the same
>>>>>>>>>> conditional. This has the advantage that you can fix the
>>>>>>>>>> comment just above it to match this special case. Do you
>>>>>>>>>> think that would work?
>>>>>>>>> Yes, that makes sense. I'll get started on it. I have a
>>>>>>>>> vacation coming up shortly, so what I'll get a new webrev out
>>>>>>>>> soon, but probably will need to wait until after my trip to do
>>>>>>>>> more thorough testing and push the changes.
>>>>>>>>>>
>>>>>>>>>> It looks like the ending "\" at line 3330 in globals.hpp
>>>>>>>>>> isn't aligned, and the copyright in cardGeneration.cpp needs
>>>>>>>>>> to be updated.
>>>>>>>>> Ok.
>>>>>>>>>>
>>>>>>>>>> One other nit, which you can ignore unless someone comes
>>>>>>>>>> forward to agree with me 8^) , is that I'd prefer the name
>>>>>>>>>> ShrinkHeapAggressively instead. Maybe this was already
>>>>>>>>>> debated elsewhere....
>>>>>>>>> The name choice hasn't really been discussed or questioned. It
>>>>>>>>> was what was suggested to me, so I stuck with it (The initial
>>>>>>>>> work was done by someone else. I'm just getting it integrated
>>>>>>>>> into 9). I can change it, although that will mean filing a new
>>>>>>>>> CCC.
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> Chris
>>>>>>>>>> Tom
>>>>>>>>>>
>>>>>>>>>> On 2/4/2016 1:36 PM, Chris Plummer wrote:
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> Please review the following for adding the -XX
>>>>>>>>>>> UseAggressiveHeapShrink option. When turned on, it tells the
>>>>>>>>>>> GC to reduce the heap size to the new target size
>>>>>>>>>>> immediately after a full GC rather than doing it
>>>>>>>>>>> progressively over 4 GCs.
>>>>>>>>>>>
>>>>>>>>>>> Webrev:
>>>>>>>>>>> http://cr.openjdk.java.net/~cjplummer/8146436/webrev.02/
>>>>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8146436
>>>>>>>>>>>
>>>>>>>>>>> Testing:
>>>>>>>>>>> -JPRT with '-testset hotspot'
>>>>>>>>>>> -JPRT with '-testset hotspot -vmflags
>>>>>>>>>>> "-XX:+UseAggressiveHeapShrink"'
>>>>>>>>>>> -added new TestMaxMinHeapFreeRatioFlags.java test
>>>>>>>>>>>
>>>>>>>>>>> thanks,
>>>>>>>>>>>
>>>>>>>>>>> Chris
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160301/c9c1b92c/attachment.htm>
More information about the hotspot-gc-dev
mailing list