<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Hi all,<br>
<br>
The webrev is updated.<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/6521376/webrev.05/">http://cr.openjdk.java.net/~tamao/6521376/webrev.05/</a><br>
<br>
The suggestions for print formatting and assertions are taken. Good
points! Thanks, Jon.<span class="moz-smiley-s1"><span> :-) </span></span><br>
<br>
Note the behaviors of -XX:+PrintTenuringDistribution<br>
<br>
(1) Parallel GC<br>
<br>
Before:<br>
java -XX:+PrintTenuringDistribution TestFullGC<br>
<br>
Desired survivor size 11010048 bytes, new threshold 7 (max 15)<br>
<br>
After:<br>
java -XX:+PrintTenuringDistribution TestFullGC<br>
<br>
Desired survivor size 524288 bytes, new threshold 7 (max threshold
15)<br>
<br>
(2) Other collectors<br>
<br>
Before:<br>
java -XX:+UseSerialGC -XX:+PrintTenuringDistribution TestFullGC<br>
<br>
Desired survivor size 65536 bytes, new threshold 1 (max 15)<br>
- age 1: 131064 bytes, 131064 total<br>
<br>
After:<br>
java -XX:+UseSerialGC -XX:+PrintTenuringDistribution TestFullGC<br>
<br>
Desired survivor size 65536 bytes, new threshold 1 (max threshold
15)<br>
- age 1: 131072 bytes, 131072 total<br>
<br>
<br>
<br>
On 11/14/13 8:32 AM, Jon Masamitsu wrote:
<blockquote cite="mid:5284FB1A.6090608@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
Tao,<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6521376/webrev.04/src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp.udiff.html">http://cr.openjdk.java.net/~tamao/6521376/webrev.04/src/share/vm/gc_implementation/parallelScavenge/psScavenge.cpp.udiff.html</a><br>
<br>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<pre><span class="new">+ gclog_or_tty->print_cr("Desired survivor size " SIZE_FORMAT " bytes, new threshold %u (max threshold %u)",</span></pre>
<br>
What do you think of using UINTX_FORMAT instead of %u? Just a
little bit future proof?<br>
<br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6521376/webrev.04/src/share/vm/gc_implementation/shared/ageTable.cpp.udiff.html">http://cr.openjdk.java.net/~tamao/6521376/webrev.04/src/share/vm/gc_implementation/shared/ageTable.cpp.udiff.html</a><br>
<br>
Consider an assertion to check that MaxTenuringThreshold has one
of the two expected values.<br>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<pre><span class="new">+ uint result;</span>
<span class="new">+</span>
<span class="new">+ if (AlwaysTenure || NeverTenure) {</span>
<span class="new">+ result = MaxTenuringThreshold;</span>
assert(MaxTenuringThreshold == 0 || MaxTenuringThreshold == <meta http-equiv="content-type" content="text/html; charset=UTF-8"><span class="removed">markOopDesc::max_age + 1,
errmsg("</span>MaxTenuringThreshold should be either 0 or " UINTX_FORMAT " and is " UINTX_FORMAT,
MaxTenuringThreshold);
<span class="new">+ } else {</span></pre>
<br>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<pre><span class="new">+ gclog_or_tty->print_cr("Desired survivor size " SIZE_FORMAT " bytes, new threshold %u (max threshold %u)",
Use UINTX_FORMAT?
That's all. Rest looks good.
Jon
</span></pre>
<br>
<div class="moz-cite-prefix">On 11/13/13 4:36 PM, Tao Mao wrote:<br>
</div>
<blockquote cite="mid:52841B22.1080300@oracle.com" type="cite">Hi
all, <br>
<br>
New webrev uploaded. <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6521376/webrev.04/">http://cr.openjdk.java.net/~tamao/6521376/webrev.04/</a>
<br>
<br>
Please see inline, Thomas. <br>
<br>
Thanks. <br>
Tao <br>
<br>
On 11/12/13 2:42 AM, Thomas Schatzl wrote: <br>
<blockquote type="cite">Hi, <br>
<br>
sorry for the late response... <br>
<br>
On Wed, 2013-11-06 at 16:43 -0800, Tao Mao wrote: <br>
<blockquote type="cite">Hi all, <br>
<br>
A new webrev is updated <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etamao/6521376/webrev.03/">http://cr.openjdk.java.net/~tamao/6521376/webrev.03/</a>
<br>
<br>
It includes a thorough test, which is one good way to
examine the <br>
changeset. <br>
</blockquote>
Agree :) <br>
<blockquote type="cite">Changeset: <br>
<br>
One point I want to discuss here is the validity of setting
<br>
-XX:MaxTenuringThreshold=16. The reason why I'd consider it
valid is: <br>
when NeverTenure is set, MaxTenuringThreshold is set to 16
while <br>
setting MaxTenuringThreshold=15 doesn't make sense here.
Thus, <br>
MaxTenuringThreshold=16 needs to be considered a valid
setting. <br>
</blockquote>
I think I am fine with this. <br>
<br>
As for the test in TestObjectTenuringFlags: I think it would
be more <br>
clear if every test were a single method call instead of the <br>
<br>
vmOpts.clear(); <br>
collections.addAll( ...test settings... ); <br>
shouldFail = ... <br>
expectedFlags = ... <br>
runTest(...) <br>
<br>
e.g. as <br>
<br>
runTest(new String[] { test settings }, true /* should_fail
*/, new <br>
ExpectedTenuringFlags( ... )); <br>
</blockquote>
Done. <br>
<blockquote type="cite"> <br>
I do not really like the use of the "vmOpts" global both in
the main() <br>
method and the ""runTenurinigFlagsConsistencyTest()". I do not
think <br>
there is need to save memory here, and that would save the
test to reset <br>
the "vmOpts" variable all the time. <br>
</blockquote>
Done. <br>
<blockquote type="cite">I do not think the test needs to
basically rethrow the RuntimeException <br>
from checkTenurinigFlagsConsistency in
checkTenurinigFlagsConsistency. <br>
The test could just let it fall through (without try-catch). <br>
</blockquote>
Done. <br>
<blockquote type="cite"> <br>
Also checkTenurinigFlagsConsistency could fail fast on any
incorrect <br>
consistency check, with an explicit message for every type of
failure <br>
(e.g. "expected AlwaysTenure to be<alwaysTenure value>,
is<actual <br>
value> instead"), instead of accumulating the error using
the&& <br>
operator. <br>
</blockquote>
Done. <br>
<blockquote type="cite"> <br>
The stack trace in the error will give you the failing test
quickly, but <br>
it's not bad to show the VM parameters that lead to the error.
<br>
</blockquote>
Chose not to do so. The stack trace should be enough to track
down. <br>
<blockquote type="cite"> <br>
There is also a typo in the "runTenurinigFlagsConsistencyTest"
test <br>
name, an additional "i". The same typo in <br>
"checkTenurinigFlagsConsistency". <br>
</blockquote>
Done. <br>
<blockquote type="cite"> <br>
Otherwise looks good. <br>
<br>
Thanks, <br>
Thomas <br>
<br>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</body>
</html>