<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Tao,<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/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);
<pre></pre>
<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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~tamao/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>
</body>
</html>