[9] RFR (S) 8146436: Add -XX:+UseAggressiveHeapShrink option

Tom Benson tom.benson at oracle.com
Tue Mar 1 18:39:29 UTC 2016


Hi Chris,
Looks good.
I do notice that the trace printed at line 273 in cardGeneration will 
show "shrink factors" of zero when ShrinkHeapInSteps is disabled, when 
100 might be expected.  But OTOH, you could interpret zero as 'not in 
use.'    I would leave it as is, but perhaps the other reviewer (or you) 
will have a different opinion.  OK with that as well.
Tom

On 3/1/2016 1:10 AM, 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?
>
> 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/f7212c84/attachment.html>


More information about the hotspot-gc-dev mailing list