<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi Chris,<br>
I agree, it makes sense to move the _shrink_factor adjustments
inside the conditional. <br>
You may as well also add the missing word (...we don't want TO
shrink...) in the comment while you're there.<br>
<br>
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^)<br>
Tom<br>
<br>
<div class="moz-cite-prefix">On 2/10/2016 3:39 PM, Chris Plummer
wrote:<br>
</div>
<blockquote cite="mid:56BB9FEA.5070506@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Tom,<br>
<br>
if (!UseAggressiveHeapShrink) {<br>
// If UseAggressiveHeapShrink is false (the default),<br>
// we don't want shrink all the way back to initSize if
people call<br>
// System.gc(), because some programs do that between
"phases" and then<br>
// we'd just have to grow the heap up again for the next
phase. So we<br>
// damp the shrinking: 0% on the first call, 10% on the
second call, 40%<br>
// on the third call, and 100% by the fourth call. But
if we recompute<br>
// size without shrinking, it goes back to 0%.<br>
shrink_bytes = shrink_bytes / 100 *
current_shrink_factor;<br>
}<br>
assert(shrink_bytes <= max_shrink_bytes, "invalid
shrink size");<br>
if (current_shrink_factor == 0) {<br>
_shrink_factor = 10;<br>
} else {<br>
_shrink_factor = MIN2(current_shrink_factor * 4,
(size_t) 100);<br>
}<br>
<br>
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.<br>
<br>
cheers,<br>
<br>
Chris<br>
<br>
On 2/10/16 12:16 PM, Tom Benson wrote:<br>
</div>
<blockquote cite="mid:56BB9A9A.7060901@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
Hi Chris,<br>
OK, that all sounds good. <br>
<br>
>> I can change it, although that will mean filing a new
CCC.<br>
Ah, I'd forgotten about that. Not worth it, unless there's a
landslide of support for a different name.<br>
<br>
Tnx,<br>
Tom<br>
<br>
<div class="moz-cite-prefix">On 2/10/2016 3:06 PM, Chris Plummer
wrote:<br>
</div>
<blockquote cite="mid:56BB9831.5030504@oracle.com" type="cite">
<meta content="text/html; charset=utf-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix">Hi Tom,<br>
<br>
Thanks for having a look. Comments inline below:<br>
<br>
On 2/10/16 11:27 AM, Tom Benson wrote:<br>
</div>
<blockquote cite="mid:56BB8F3D.3070502@oracle.com" type="cite">Hi
Chris, <br>
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. <br>
</blockquote>
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
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
MaxHeapFreeRatio is fully honored. If this is what you are
looking for, I can add it to the CR.<br>
<blockquote cite="mid:56BB8F3D.3070502@oracle.com" type="cite">
<br>
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? <br>
</blockquote>
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.<br>
<blockquote cite="mid:56BB8F3D.3070502@oracle.com" type="cite">
<br>
It looks like the ending "\" at line 3330 in globals.hpp
isn't aligned, and the copyright in cardGeneration.cpp needs
to be updated. <br>
</blockquote>
Ok.<br>
<blockquote cite="mid:56BB8F3D.3070502@oracle.com" type="cite">
<br>
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.... <br>
</blockquote>
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.<br>
<br>
thanks,<br>
<br>
Chris<br>
<blockquote cite="mid:56BB8F3D.3070502@oracle.com" type="cite">Tom
<br>
<br>
On 2/4/2016 1:36 PM, Chris Plummer wrote: <br>
<blockquote type="cite">Hello, <br>
<br>
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. <br>
<br>
Webrev: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ecjplummer/8146436/webrev.02/">http://cr.openjdk.java.net/~cjplummer/8146436/webrev.02/</a>
<br>
Bug: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8146436">https://bugs.openjdk.java.net/browse/JDK-8146436</a>
<br>
<br>
Testing: <br>
-JPRT with '-testset hotspot' <br>
-JPRT with '-testset hotspot -vmflags
"-XX:+UseAggressiveHeapShrink"' <br>
-added new TestMaxMinHeapFreeRatioFlags.java test <br>
<br>
thanks, <br>
<br>
Chris <br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>