<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
New webrev:<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/6761744/webrev.03/">http://cr.openjdk.java.net/~tamao/6761744/webrev.03/</a><br>
<br>
jtreg test passed.<br>
<br>
See inline.<br>
<br>
Thanks.<br>
Tao<br>
<br>
On 4/22/13 12:58 AM, Bengt Rutisson wrote:
<blockquote cite="mid:5174EDA3.9000605@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Tao,<br>
<br>
On 4/19/13 7:27 PM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:51717E6E.4020806@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
1. check overflow routine is wrapped up.<br>
</blockquote>
<br>
I would prefer that the method
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
CollectedHeap::add_to_total_reserved_and_check_overflow() did not
return a bool. Since you do vm_exit_during_initialization() when
you overflow there is no need to return different results. This
would also simplify the code where you use it. No need to use if
statements.<br>
</blockquote>
It's fine to remove the bool return type. Done.<br>
<blockquote cite="mid:5174EDA3.9000605@oracle.com" type="cite"> <br>
Also, I think you should remove the naming references to
"total_reserved" and "reserved". This method is general enough to
just add two size_t arguments together and check for overflow.
Especially if it would take the message as a parameter. Instead of
returning a bool you can return the result of the addition if
there was no overflow.<br>
</blockquote>
I'd like not to make the method generic for (1) it's not an
appropriate place to host such a generic add_to_and_check_overflow
method; (2) Remember, the refactoring was originally suggested to
reduce duplication of error message. Passing the error message gets
back the flaw.
<blockquote cite="mid:5174EDA3.9000605@oracle.com" type="cite"> <br>
I'm not sure we need round_up_total_reserved_and_check_overflow()
since it is only being used in one place. <br>
</blockquote>
The same reason: avoid a duplication of string definition of error
message.<br>
<blockquote cite="mid:5174EDA3.9000605@oracle.com" type="cite"> <br>
<blockquote cite="mid:51717E6E.4020806@oracle.com" type="cite"> <br>
2. The reduction of code duplication is adopted.<br>
</blockquote>
<br>
Good.<br>
<br>
<blockquote cite="mid:51717E6E.4020806@oracle.com" type="cite"> <br>
3. Develop a way to intake 0~multiple external vm options in
jtreg main().</blockquote>
<br>
Yes, this looks like it should work. However, based on John
Cuthbertson's discussion with Leonid I think you need to do the
same thing for test.java.opts too.<br>
</blockquote>
Followed the Leonid's latest suggestion.<br>
<blockquote cite="mid:5174EDA3.9000605@oracle.com" type="cite"> <br>
Bengt<br>
<br>
<br>
<br>
<blockquote cite="mid:51717E6E.4020806@oracle.com" type="cite">
Hope it helps others implement similar functionality.<br>
webrev:<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6761744/webrev.02/">http://cr.openjdk.java.net/~tamao/6761744/webrev.02/</a><br>
<br>
test:<br>
JTREG: passed.<br>
jtreg -jdk:/home/tamao/jdk1.7.0_04-i586 -vmoptions:"-tamao
<GC_OPTION>"
/home/tamao/src/6761744CrashIfProcessSizeLimitExceeded_hsx24/test/gc/init/TestHandleExceedingProcessSizeLimitIn32BitBuilds.java<br>
<br>
where GC_OPTION rotates in -XX:+UseParallelGC -XX:+UseG1GC
-XX:+UseSerialGC -XX:+UseParNewGC -XX:+UseConcMarkSweepGC<br>
<br>
<br>
Thanks.<br>
Tao<br>
<br>
On 4/15/13 1:37 AM, Bengt Rutisson wrote:
<blockquote cite="mid:516BBC64.6060509@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Tao,<br>
<br>
I agree with John. The changes look fine and I'm ok with
them. But it would look nicer to add the
CollectedHeap::add_and_verify_no_overflow() method.<br>
<br>
Also, regarding the test, a couple of minor things.<br>
<br>
First, with the new naming convention I think the test
should be called something like:<br>
<br>
test/gc/init/TestHandleExceedingProcessSizeLimitOn32BitSystems.java<br>
<br>
Then I think you can reduce the code duplication a bit. You
create the same ProcessBuilder and OutputAnalyzer in both 32
and 64 bit cases. So maybe it could look like this instead:<br>
<br>
<tt>public class
TestHandleExceedingProcessSizeLimitOn32BitSystems {</tt><tt><br>
</tt><tt> public static void main(String args[]) throws
Exception {</tt><tt><br>
</tt><tt> ProcessBuilder pb =</tt><tt><br>
</tt><tt>
ProcessTools.createJavaProcessBuilder(System.getProperty("test.vm.opts"),</tt><tt><br>
</tt><tt>
"-Xmx3072m",</tt><tt><br>
</tt><tt>
"-XX:MaxPermSize=1024m",</tt><tt><br>
</tt><tt>
"-version");</tt><tt><br>
</tt><tt> OutputAnalyzer output = new
OutputAnalyzer(pb.start());</tt><tt><br>
</tt><tt><br>
</tt><tt> String dataModel =
System.getProperty("sun.arch.data.model");</tt><tt><br>
</tt><tt> if (dataModel.equals("32")) {</tt><tt><br>
</tt><tt> output.shouldContain("The size of the object
heap + perm gen exceeds the maximum representable size");</tt><tt><br>
</tt><tt> if (output.getExitValue() == 0) {</tt><tt><br>
</tt><tt> throw new RuntimeException("Not expected to
get exit value 0");</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt> } else if (dataModel.equals("64")) {</tt><tt><br>
</tt><tt> output.shouldHaveExitValue(0);</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt> }</tt><tt><br>
</tt><tt>}</tt><tt><br>
</tt><br>
There is also a small bug in the test. If you run JTreg
without passing any vmoptions the test will fail. The reason
is that System.getProperty("test.vm.opts") will evaluate to
the empty string. This will be passed as the first argument
to the Java process, which will assume that this is the
class name. So, it fails to start because it can't load the
class <empty string>.<br>
<br>
To reproduce the problem use this command line:<br>
<br>
java -jar <path_to_jtreg>/lib/jtreg.jar
test/gc/6761744/TestHandleExceedingProcessSizeLimitOn32BitSystems.java<br>
<br>
I guess the fix is to check that
System.getProperty("test.vm.opts") is not empty before
passing it on to createJavaProcessBuilder().<br>
<br>
Thanks,<br>
Bengt<br>
<br>
On 4/13/13 3:15 AM, John Cuthbertson wrote:<br>
</div>
<blockquote cite="mid:5168B1AD.7060302@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hi Tao,<br>
<br>
This looks OK to me.<br>
<br>
I can see what Thomas is saying though. All of the checks
involve something like:<br>
<br>
total += some_value;<br>
if (total < some_value) {<br>
// We must have overflowed<br>
vm_exit(...);<br>
}<br>
<br>
So a function in CollectedHeap (the base class of SharedHeap
and ParallelScavengeHeap) might make sense. For example:<br>
<br>
total_reserved = 0;<br>
total_reserved = add_and_verify_no_overflow(total_reserved,
max_heap_size);<br>
total_reserved = add_and_verify_no_overflow(total_reserved,
max_perm_size);<br>
<br>
Where the function is:<br>
<br>
size_t add_and_verify_no_overflow(size_t x, size_t y) {<br>
const char* msg = "...";<br>
x += y;<br>
if (x < y) {<br>
vm_exit(msg);<br>
}<br>
return x;<br>
}<br>
<br>
But I can live with your current changes.<br>
<br>
JohnC<br>
<br>
<div class="moz-cite-prefix">On 4/10/2013 9:52 AM, Tao Mao
wrote:<br>
</div>
<blockquote cite="mid:516598D3.1020809@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
Hi Bengt,<br>
<br>
Thank you for reviewing. A new webrev is updated.<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6761744/webrev.01/">http://cr.openjdk.java.net/~tamao/6761744/webrev.01/</a><br>
<br>
Cheers,<br>
Tao<br>
<br>
On 4/10/13 1:54 AM, Bengt Rutisson wrote:
<blockquote cite="mid:516528C2.8080709@oracle.com"
type="cite">
<meta content="text/html; charset=UTF-8"
http-equiv="Content-Type">
<div class="moz-cite-prefix"><br>
Hi Tao,<br>
<br>
This change looks good. Thanks for adding the JTReg
test, it looks good!<br>
<br>
One minor nit:<br>
<br>
In
src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
I would suggest to
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
store "(size_t) align_size_up(pgs->max_size(),
HeapRegion::GrainBytes)" in a local variable rather
than duplicating the calculation.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
On 4/8/13 7:22 AM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:51625424.4050209@oracle.com"
type="cite">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
6761744 Hotspot crashes if process size limit is
exceeded<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://jbs.oracle.com/bugs/browse/JDK-6761744">https://jbs.oracle.com/bugs/browse/JDK-6761744</a><br>
<br>
webrev:<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6761744/webrev.00/">http://cr.openjdk.java.net/~tamao/6761744/webrev.00/</a><br>
<br>
changeset:<br>
The fix only needs to go to hsx24 since there's no
perm gen in hotspot-25. Thus, the webrev is based on
hsx24 repo.<br>
<br>
It provides for 32-bit builds a preventive check of
the size of "the object heap + perm gen" before
reserving VM memory. The total size should not exceed
4096MB (i.e. 4GB) for 32-bit builds; otherwise, the
total doesn't make sense and, what's worse, overflow
occurs. It will consequentially trigger anther error
of memory access violation, which was not protected<span
style="color: rgb(0, 0, 0); font-family: Arial,
FreeSans, Helvetica, sans-serif; font-size:
12.727272033691406px; font-style: normal;
font-variant: normal; font-weight: normal;
letter-spacing: normal; line-height:
15.454545021057129px; orphans: auto; text-align:
start; text-indent: 0px; text-transform: none;
white-space: normal; widows: auto; word-spacing:
0px; -webkit-text-size-adjust: auto;
-webkit-text-stroke-width: 0px; background-color:
rgb(240, 240, 240); display: inline !important;
float: none;"></span>.<br>
<br>
jtreg testing java code is also written, checking both
32-bit and (trivially) 64-bit builds.<br>
<br>
testing:<br>
check jtreg tests with flags -XX:+UseParallelGC,
-XX:+UseG1GC, -XX:+UseParNewGC,
-XX:+UseConcMarkSweepGC, -XX:+UseSerialGC and builds
of 32-bit and 64-bit. All passed.<br>
<br>
Needs JPRT test when pushing.<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</body>
</html>