<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi there,<br>
<br>
If the avg scheme does something reasonable when only -Xmx is set
(to something large), I'm totally OK with it. I think I confused
initial / min size when I read your previous e-mail (still fetching
that stuff from cold long-term storage, it's taking some time).<br>
<br>
For folks who set both -Xms and/or -Xmx and/or G1HeapRegionSize you
might want to consider generating a warning when the settings look
suspect, if you don't want to directly change them. But, if you want
to do that, a separate CR should be fine with it; it's kind of
orthogonal to this fix.<br>
<br>
I also looked at the webrev and it looks fine. Out of curiosity:<br>
<br>
g1CollectorPolicy.cpp:<br>
<meta charset="utf-8">
<pre style="color: rgb(0, 0, 0); font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; background-color: rgb(238, 238, 238);"> 170 // Set up the region size and associated fields. Given that the
171 // policy is created before the heap, we have to set this up here,
<span class="changed" style="color: blue;"> 172 // so it's done as soon as possible</span>
</pre>
Why did you remove the period after possible?<br>
<br>
Tony<br>
<br>
<div class="moz-cite-prefix">On 8/29/13 10:21 AM, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:521F58F4.5020802@oracle.com" type="cite">On
8/29/13 4:14 PM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
Tony,
<br>
<br>
On 8/29/13 4:12 PM, Tony Printezis wrote:
<br>
<blockquote type="cite">Bengt,
<br>
<br>
Doesn't what I said also applies for users who do not set
-Xms? So, if -Xms is by default 6m, a user launches the VM
with:
<br>
<br>
java -Xmx32G ...
<br>
<br>
and the region size is calculated to be 8m, what's the initial
heap size (8m, i.e. one region)?
<br>
</blockquote>
<br>
No, by default we calculate a reasonable -Xms. So as long as the
policy uses that value and not the min heap size I think we are
fine.
<br>
</blockquote>
<br>
Just to clarify a bit. In your example we get a -Xms of 128m:
<br>
<br>
java -XX:+UseG1GC -XX:+PrintFlagsFinal -Xmx32G -version | grep
InitialHeapSize
<br>
uintx InitialHeapSize :=
134217728 {product}
<br>
<br>
<br>
With my fix this means that we will get a region size based on
128m + 32g / 2, which will turn out to be 8m. That gives us a
minimum of 16 regions (cool coincidence since that's what you
suggested ;) ). And it will give us a maximum of about 4000
regions.
<br>
<br>
Before my fix we would get a regions size of 1m, which means about
32000 regions.
<br>
<br>
Bengt
<br>
<br>
<br>
<br>
<blockquote type="cite">
<br>
Bengt
<br>
<br>
<blockquote type="cite">
<br>
Tony
<br>
<br>
On 8/29/13 10:08 AM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
Hi Tony,
<br>
<br>
Thanks for looking at this!
<br>
<br>
Comments inline.
<br>
<br>
On 8/29/13 3:03 PM, Tony Printezis wrote:
<br>
<blockquote type="cite">Hi Bengt,
<br>
<br>
Yeah, I struggled with this heuristic when I did the
original implementation of the heap region calculation.
The issue only arises when the gap between the min and max
heap size is very large. So, if someone launches the VM
with:
<br>
<br>
java -Xms32m -Xmx64g ...
<br>
<br>
and G1 picks a region size of 8m, it'd start with only 4
regions which will probably make performance right at the
beginning will be terrible (but I agree that it will be
better as the heap grows, compared to if an 1m region size
was used).
<br>
</blockquote>
<br>
Agreed. And just to be clear. The main problem with the
existing policy is that it by default always picks 1m
regions if nothing is set on the command line. This is due
to the fact that it is not based on the initial heap size
(-Xms) but on the min heap size, which by default is in the
order of 6m. So, those who set -Xms on the command line have
experienced less of a problem. At least if they set -Xms to
high enough values.
<br>
<br>
<blockquote type="cite">
<br>
Can I suggest maybe an additional policy change? Use the
avg to calculate the region size, as you proposed, but
potentially adjust the min heap size based on a min region
number (let's pick a number of a hat: 16; you might want
to revise this). So, in the above example:
<br>
<br>
-Xms32m -Xmx64g -> region size = 8m
<br>
<br>
you'll actually adjust the min heap size 16 x 8m = 128m.
This will avoid the potentially bad behavior right at the
start. Of course, you'll start with a larger heap size
than what the user asked for. On the other hand, if
someone uses a huge max they probably expect the heap to
grow. So starting with a large min might be OK.
<br>
</blockquote>
<br>
I see your point, but I don't really like the fact that if
someone explicitly sets -Xms on the command line we would
ignore that and use a value that is four times as large.
Also, there is the possibility to set the region size using
G1HeapRegionSize on the command line. So, in this use case I
kind of think it would be better to leave it up to the user
to indicate if the heap is more likely to be 32m or 64g by
setting the region size explicitly.
<br>
<br>
Thanks,
<br>
Bengt
<br>
<br>
<blockquote type="cite">
<br>
Tony
<br>
<br>
On 8/29/13 5:26 AM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
Hi everyone,
<br>
<br>
Could I have a couple of reviews of this change:
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~brutisso/8019902/webrev.00/">http://cr.openjdk.java.net/~brutisso/8019902/webrev.00/</a>
<br>
<br>
The fact that G1 by default bases its region size on the
minimum heap size means that out of the box the region
size will always be 1M. This is a problem on large
machines with lots of memory. We pick a large heap size
but get a very small region size. The small regions are
inefficient and cause a lot of memory footprint.
Normally we aim to get around 2048 regions, but on a
machine with a lot of memory we might pick a default max
heap size of 32G, which means that we will get ~32000
regions. This can lead to out of memory situations -
especially on Solaris x86.
<br>
<br>
This patch changes the heuristics for picking the region
size to use the average between initial heap size (-Xms)
and the maximum heap size (-Xmx). This means that for
large heaps we will pick larger region sizes. In the 32G
example we will now pick a region size of 8m which means
that we will have 4000 regions which is more reasonable.
<br>
<br>
Thanks,
<br>
Bengt
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Tony Printezis | Staff Software Engineer | Twitter
@TonyPrintezis
<a class="moz-txt-link-abbreviated" href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a>
</pre>
</body>
</html>