<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/16/13 1:45 PM, Jesper Wilhelmsson
wrote:<br>
</div>
<blockquote cite="mid:50F6A0DA.4040108@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<br>
<div class="moz-cite-prefix">On 2013-01-16 09:23, Bengt Rutisson
wrote:<br>
</div>
<blockquote cite="mid:50F66391.6000504@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<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>
</blockquote>
<br>
Oh, yes, you're right. Sorry!<br>
<br>
Good catch Vitaly!<br>
<br>
New webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Ejwilhelm/6348447/webrev.3">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>
</blockquote>
<br>
It is not enough to check NewRatio != 0 since it is a signed value.
You should check NewRatio > 0. Or change the declaration of
NewRatio from intx to uintx.<br>
<br>
As it is now I think you will get a really huge result from
recommended_heap_size() if someone sets -XX:NewRatio=-10 on the
command line since you will be converting negative integers to a
size_t value.<br>
<br>
As I mentioned before the NewRatio > 0 check is not done for G1.
But this should probably be considered a separate bug. On the other
hand if we could make G1 inherit from GenCollectorPolicy it would do
the NewRatio > 0 check and we would also have a better place to
put the OldSize checks that you want to add. Two bugs for the price
of one ;)<br>
<br>
> java -XX:+UseG1GC -XX:NewRatio=-1 -version<br>
#<br>
# A fatal error has been detected by the Java Runtime Environment:<br>
#<br>
# SIGFPE (0x8) at pc=0x000000010f5468a8, pid=31206, tid=4611<br>
#<br>
# JRE version: (8.0-b68) (build )<br>
# Java VM: Java HotSpot(TM) 64-Bit Server VM (25.0-b16-internal-jvmg
mixed mode bsd-amd64 compressed oops)<br>
# Problematic frame:<br>
# V [libjvm.dylib+0x6618a8]
G1YoungGenSizer::heap_size_changed(unsigned int)+0x14a<br>
#<br>
# Failed to write core dump. Core dumps have been disabled. To
enable core dumping, try "ulimit -c unlimited" before starting Java
again<br>
#<br>
# An error report file with more information is saved as:<br>
# /Users/brutisso/repos/hs-gc/hs_err_pid31206.log<br>
#<br>
# If you would like to submit a bug report, please visit:<br>
# <a class="moz-txt-link-freetext" href="http://bugreport.sun.com/bugreport/crash.jsp">http://bugreport.sun.com/bugreport/crash.jsp</a><br>
#<br>
Current thread is 4611<br>
Dumping core ...<br>
Abort trap: 6<br>
<br>
<br>
Bengt<br>
<br>
<blockquote cite="mid:50F6A0DA.4040108@oracle.com" type="cite">
/Jesper<br>
<br>
<br>
<blockquote cite="mid:50F66391.6000504@oracle.com" type="cite">
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>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>