[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: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20160301/c9c1b92c/attachment.html>


More information about the hotspot-gc-dev mailing list