<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 1/15/13 2:41 PM, Jesper Wilhelmsson
wrote:<br>
</div>
<blockquote cite="mid:50F55C7A.8070508@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
On 2013-01-15 14:32, Vitaly Davidovich wrote:<br>
<blockquote
cite="mid:CAHjP37GPhmZzGrPUkiaeuabQRq3o1YtNXnj+4-Y4RD25+Vb-gQ@mail.gmail.com"
type="cite">
<p dir="ltr">Hi Jesper,</p>
<p dir="ltr">Is NewRatio guaranteed to be non-zero when used
inside recommended_heap_size?</p>
</blockquote>
As far as I can see, yes. It defaults to two and is never set to
zero.<br>
</blockquote>
<br>
No, there is no such guarantee this early in the argument parsing.
The check to verify that NewRatio > 0 is done in
GenCollectorPolicy::initialize_flags(), which is called later in the
start up sequence than your call to
CollectorPolicy::recommended_heap_size() and it is never called for
G1.<br>
<br>
Running with your patch crashes:<br>
<br>
java -XX:OldSize=128m -XX:NewRatio=0 -version<br>
Floating point exception: 8<br>
<br>
Bengt<br>
<blockquote cite="mid:50F55C7A.8070508@oracle.com" type="cite">
/Jesper<br>
<br>
<blockquote
cite="mid:CAHjP37GPhmZzGrPUkiaeuabQRq3o1YtNXnj+4-Y4RD25+Vb-gQ@mail.gmail.com"
type="cite">
<p dir="ltr">Thanks</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Jan 15, 2013 8:11 AM, "Jesper
Wilhelmsson" <<a moz-do-not-send="true"
href="mailto:jesper.wilhelmsson@oracle.com">jesper.wilhelmsson@oracle.com</a>>
wrote:<br type="attribution">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"> Jon,<br>
<br>
Thank you for looking at this! I share your concerns and I
have moved the knowledge about policies to CollectorPolicy.
set_heap_size() now simply asks the collector policy if it
has any recommendations regarding the heap size.<br>
<br>
Ideally, since the code knows about young and old
generations, I guess the new function
"recommended_heap_size()" should be placed in
GenCollectorPolicy, but then the code would have to be
duplicated for G1 as well. However, CollectorPolicy already
know about OldSize and NewSize so I think it is OK to put it
there.<br>
<br>
Eventually I think that we should reduce the abstraction
level in the generation policies and merge CollectorPolicy,
GenCollectorPolicy and maybe even
TwoGenerationCollectorPolicy and if possible
G1CollectorPolicy, so I don't worry too much about having
knowledge about the two generations in CollectorPolicy.<br>
<br>
<br>
A new webrev is available here:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejwilhelm/6348447/webrev.2/"
target="_blank">http://cr.openjdk.java.net/~jwilhelm/6348447/webrev.2/</a><br>
<br>
Thanks,<br>
/Jesper<br>
<br>
<br>
<br>
On 2013-01-14 19:00, Jon Masamitsu wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"> Jesper,<br>
<br>
I'm a bit concerned that set_heap_size() now knows about
how<br>
the CollectorPolicy uses OldSize and NewSize. In the
distant<br>
past set_heap_size() did not know what kind of collector
was<br>
going to be used and probably avoided looking at those<br>
parameters for that reason. Today we know that a
generational<br>
collector is to follow but maybe you could hide that
knowledge<br>
in CollectorPolicy somewhere and have set_heap_size() call
into<br>
CollectorPolicy to use that information?<br>
<br>
Jon<br>
<br>
<br>
On 01/14/13 09:10, Jesper Wilhelmsson wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex"> Hi,<br>
<br>
I would like a couple of reviews of a small fix for
JDK-6348447 - Specifying -XX:OldSize crashes 64-bit VMs<br>
<br>
Webrev:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejwilhelm/6348447/webrev/"
target="_blank">http://cr.openjdk.java.net/~jwilhelm/6348447/webrev/</a><br>
<br>
Summary:<br>
When starting HotSpot with an OldSize larger than the
default heap size one will run into a couple of
problems. Basically what happens is that the OldSize is
ignored because it is incompatible with the heap size. A
debug build will assert since a calculation on the way
results in a negative number, but since it is a size_t
an if(x<0) won't trigger and the assert catches it
later on as incompatible flags.<br>
<br>
Changes:<br>
I have made two changes to fix this.<br>
<br>
The first is to change the calculation in
TwoGenerationCollectorPolicy::adjust_gen0_sizes so that
it won't result in a negative number in the if
statement. This way we will catch the case where the
OldSize is larger than the heap size and adjust the
OldSize instead of the young size. There are also some
cosmetic changes here. For instance the argument
min_gen0_size is actually used for the old generation
size which was a bit confusing initially. I renamed it
to min_gen1_size (which it already was called in the
header file).<br>
<br>
The second change is in Arguments::set_heap_size. My
reasoning here is that if the user sets the OldSize we
should probably adjust the heap size to accommodate that
OldSize instead of complaining that the heap is too
small. We determine the heap size first and the
generation sizes later on while initializing the VM. To
be able to fit the generations if the user specifies
sizes on the command line we need to look at the
generation size flags a little already when setting up
the heap size.<br>
<br>
Thanks,<br>
/Jesper<br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</div>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>