<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 2013-01-16 14:23, Vitaly Davidovich wrote:<br>
<blockquote
cite="mid:CAHjP37HtKTM522d6_P_OmvG1wb0iSyKxtX7LXuV7XrNC0EKMdw@mail.gmail.com"
type="cite">
<p dir="ltr">Looks good Jesper. Maybe just a comment there that
NewRatio hasn't been checked yet but if it's 0, VM will exit
later on anyway - basically, what you said in the email :).</p>
</blockquote>
<br>
Yes, I'll add a comment about that.<br>
Thanks,<br>
/Jesper<br>
<br>
<blockquote
cite="mid:CAHjP37HtKTM522d6_P_OmvG1wb0iSyKxtX7LXuV7XrNC0EKMdw@mail.gmail.com"
type="cite">
<p dir="ltr">Cheers</p>
<p dir="ltr">Sent from my phone</p>
<div class="gmail_quote">On Jan 16, 2013 7:49 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">
<div text="#000000" bgcolor="#FFFFFF"> <br>
<div>On 2013-01-16 09:23, Bengt Rutisson wrote:<br>
</div>
<blockquote type="cite">
<div>On 1/15/13 2:41 PM, Jesper Wilhelmsson wrote:<br>
</div>
<blockquote type="cite"> On 2013-01-15 14:32, Vitaly
Davidovich wrote:<br>
<blockquote 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>
</blockquote>
<br>
Oh, yes, you're right. Sorry!<br>
<br>
Good catch Vitaly!<br>
<br>
New webrev:<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Ejwilhelm/6348447/webrev.3"
target="_blank">http://cr.openjdk.java.net/~jwilhelm/6348447/webrev.3</a><br>
<br>
I'm just skipping the calculation if NewRatio is zero. The
VM will abort anyway as soon as it realizes that this is the
case.<br>
/Jesper<br>
<br>
<br>
<blockquote type="cite"> Bengt<br>
<blockquote type="cite"> /Jesper<br>
<br>
<blockquote 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"
target="_blank">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>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>