<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hi Tony,<br>
<br>
No worries about the delay, I'm also quite busy with other things
here. ;)<br>
<br>
<div class="moz-cite-prefix">On 2015-06-24 20:41, Tony Printezis
wrote:<br>
</div>
<blockquote
cite="mid:etPan.558af9e9.4affa36.1215a@tw-mbp-tprintezis"
type="cite">
<style>body{font-family:Helvetica,Arial;font-size:13px}</style>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;">New webrev
that covers all the suggestions:</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br>
</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><a
moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Etonyp/8086056/webrev.1/">http://cr.openjdk.java.net/~tonyp/8086056/webrev.1/</a></div>
</blockquote>
<br>
Thanks for providing a new webrev!<br>
<br>
Some comments:<br>
<br>
parNewGeneration.cpp<br>
<br>
The flag validation code on lines 886-912 can be replaced by the
recently added support for command line verification. See JEP-245:<br>
<br>
JEP 245: Validate JVM Command-Line Flag Arguments<br>
<a class="moz-txt-link-freetext" href="http://openjdk.java.net/jeps/245">http://openjdk.java.net/jeps/245</a><br>
<a class="moz-txt-link-freetext" href="http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/5bbf25472731">http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/5bbf25472731</a><br>
<br>
You can simply specify a constraint
function in the definition of the flags in globals.hpp.<br>
<br>
<br>
I have a very hard time following all the type conversions going on
in adjust_cards_per_stride().<br>
<br>
The capacity flags are size_t, the DynamicParGCStrides[Min/Max]Size
flags are uintx, we then convert the result through intptr_t, int
and uintx to eventually assign back to ParGCCardsPerStrideChunk as
intx. The value of ParGCCardsPerStrideChunk is then used in the code
to offset pointers of the jbyte* type. How can we be sure that this
works correctly? <br>
<br>
<br>
globalDefinitions.hpp<br>
<br>
I don't think that globalDefinitions.hpp is the right place to put
linearly_interpolate_bounded(). I would prefer to have it as a local
method in parNewGeneration.cpp. Or if you think it is actually going
to be used more widely I think it should go somewhere in the
utilities/ directory. <br>
<br>
Does it really make sense to use different types for X and Y in
linearly_interpolate_bounded()? I would think that one type is
enough. Also, it does not look safe to just cast the type to double
for a general purpose template method.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote
cite="mid:etPan.558af9e9.4affa36.1215a@tw-mbp-tprintezis"
type="cite">
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br>
</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;">(and I also
found a version of hg that works OK with webrev; thanks to Jon
Masa for the help)</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br>
</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;">I’m not really
happy with the way I deal with the 32-bit case. Better
suggestions are welcome. Also, I’m not attached to what the cmd
line args are called. Better suggestions on that are welcome
too.</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;"><br>
</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px; color:
rgba(0,0,0,1.0); margin: 0px; line-height: auto;">Tony</div>
<br>
<p class="airmail_on" style="color:#000;">On June 23, 2015 at
12:19:12 PM, Tony Printezis (<a moz-do-not-send="true"
href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a>)
wrote:</p>
<blockquote type="cite" class="clean_bq"><span>
<div style="word-wrap: break-word; -webkit-nbsp-mode: space;
-webkit-line-break: after-white-space;">
<div>
<title></title>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px;
color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
Hi Bengt,</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px;
color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
<br>
</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px;
color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
I’ll try factoring out the interpolation and tell me
what you
think. Sorry, I’m a bit behind on this as I’ve been busy
with a
couple of more urgent tasks hee. I’ll get this done
shortly.</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px;
color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
<br>
</div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px;
color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">
Tony</div>
<br>
<p class="airmail_on" style="color:#000;">On June 22, 2015
at
3:36:10 AM, Bengt Rutisson (<a moz-do-not-send="true"
href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>)
wrote:</p>
<blockquote type="cite" class="clean_bq">
<div bgcolor="#FFFFFF" text="#000000">
<div><span><br>
Hi Tony,<br>
<br>
I agree with your comments. Thanks for fixing
this!<br>
<br>
One answer at the end of this message...<br>
<br>
</span>
<div class="moz-cite-prefix"><span>On 2015-06-18
15:50, Tony
Printezis wrote:<br>
</span></div>
<blockquote
cite="mid:etPan.5582cc8d.1bcb7375.1215a@tw-mbp-tprintezis"
type="cite">
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px;
color: rgba(0,0,0,1.0); margin: 0px;
line-height: auto;">
<span>Hi Bengt,</span></div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px;
color: rgba(0,0,0,1.0); margin: 0px;
line-height: auto;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family:Helvetica,Arial;font-size:13px;
color: rgba(0,0,0,1.0); margin: 0px;
line-height: auto;">
<span>Thanks for looking at it. Inline.</span></div>
<span><br>
</span>
<p class="airmail_on" style="color:#000;"><span>On
June 18, 2015 at
3:02:45 AM, Bengt Rutisson (<a
moz-do-not-send="true"
href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>)
wrote:</span></p>
<div>
<blockquote type="cite" class="clean_bq"
style="color: rgb(0, 0, 0); font-family:
Helvetica, Arial; font-size: 13px; font-style:
normal; font-variant: normal; font-weight:
normal; letter-spacing: normal; line-height:
normal; orphans: auto; text-align: start;
text-indent: 0px; text-transform: none;
white-space: normal; widows: auto;
word-spacing: 0px; -webkit-text-stroke-width:
0px;">
<div bgcolor="#FFFFFF" text="#000000">
<div>
<div class="moz-cite-prefix"><span><span><br>
Hi Tony,<br>
<br>
NIce to hear from you!<br>
<br>
<br>
On 18/06/15 00:30, Tony Printezis
wrote:<br>
</span></span></div>
<blockquote
cite="mid:etPan.5581f51b.699a1ea0.1215a@tw-mbp-tprintezis"
type="cite">
<div id="bloop_customfont"
style="font-family: Helvetica, Arial;
font-size: 13px; color: rgb(0, 0, 0);
margin: 0px;">
<span>Hi all,</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica, Arial;
font-size: 13px; color: rgb(0, 0, 0);
margin: 0px;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica, Arial;
font-size: 13px; color: rgb(0, 0, 0);
margin: 0px;">
<span>A small patch for your
consideration:</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica, Arial;
font-size: 13px; color: rgb(0, 0, 0);
margin: 0px;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica, Arial;
font-size: 13px; color: rgb(0, 0, 0);
margin: 0px;">
<span><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Etonyp/8086056/webrev.0/">http://cr.openjdk.java.net/~tonyp/8086056/webrev.0/</a></span></div>
</blockquote>
<span><br>
A couple of style comments:<br>
<br>
891 const size_t MinOldGenCapacity
= G;<br>
892 const size_t MaxOldGenCapacity =
16 *
G;<br>
893 assert(MinOldGenCapacity <=
MaxOldGenCapacity, "sanity");<br>
<br>
Local variables are normmaly named using
lower case and
underscore.<span
class="Apple-converted-space"> </span></span></div>
</div>
</blockquote>
</div>
<p><br>
</p>
<p>Yeah, I of course usually do that. But as they
are constants I
thought I’d make them look like constants. :-)</p>
<p><br>
</p>
<div>
<div>
<div>
<blockquote type="cite" class="clean_bq"
style="color: rgb(0, 0, 0); font-family:
Helvetica, Arial; font-size: 13px;
font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing:
normal; line-height: normal; orphans:
auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal;
widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div bgcolor="#FFFFFF" text="#000000">
<div><span>So, min_old_gen_capacity and
max_old_gen_capacity. Same
for the rest of the local variables
in
ParNewGeneration::adjust_cards_per_stride().<br>
<br>
Having said that, I think it would
be good to turn some of these
constants into flags (which means
that the naming should be as it
is now ;) ).<span
class="Apple-converted-space"> </span></span></div>
</div>
</blockquote>
</div>
<p><br>
</p>
<p>Hang on! Are you and Masa encourating me to
add 5 (!!!) more cmd
line args to HotSpot? ;-)</p>
<p><br>
</p>
<div>
<blockquote type="cite" class="clean_bq"
style="color: rgb(0, 0, 0); font-family:
Helvetica, Arial; font-size: 13px;
font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing:
normal; line-height: normal; orphans:
auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal;
widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div bgcolor="#FFFFFF" text="#000000">
<div><span>Especially since you say that
you haven't done much
performance testing of these values.</span></div>
</div>
</blockquote>
</div>
</div>
<p><br>
</p>
<p>To be clear: I did performance testing with
chunks sizes in the
256-8k range and with three old gen sizes (2g,
10g, and 20g) and
auto-tuning comes pretty close to optimal; I
have numbers on the
JIRA. But, yes, that was only with a couple of
(synthetic) tests
and only on one architecture (Linux/x64). If
someone there could
try this on a couple more apps and maybe on
sparc, as we don’t
happen to have any sparc boxes lying around
:-), it’d be
helpful.</p>
<p>But, yes, your suggestion of making the chunk
size / old gen
capacity limits into cmd line args definitely
makes sense and I had
also considered that myself. But I had decided
against it as I
thought you wouldn’t want more cmd line args.
:-) How about:</p>
<p>+UseDynamicParGCStrides</p>
<p>DynamicParGCStrides{Min,Max}OldGenCapacity</p>
<p>DynamicParGCStrides{Min,Max}Size</p>
<p>I should also make them all manageable.</p>
<p><br>
</p>
<div>
<div>
<blockquote type="cite" class="clean_bq"
style="color: rgb(0, 0, 0); font-family:
Helvetica, Arial; font-size: 13px;
font-style: normal; font-variant: normal;
font-weight: normal; letter-spacing:
normal; line-height: normal; orphans:
auto; text-align: start; text-indent: 0px;
text-transform: none; white-space: normal;
widows: auto; word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div bgcolor="#FFFFFF" text="#000000">
<div><span><span
class="Apple-converted-space"> </span>I
would
prefer that we could adjust them at
runtime. Best would of course
be if you could take the time to do
the performance
testing.</span></div>
</div>
</blockquote>
</div>
<p><br>
</p>
<p>So, as I said earlier, I did. The numbers
are on the JIRA. Is
there anything else I could maybe run? Maybe
larger old gen than
20g? (but that’s reaching the memory limits
of my workstation,
FWIW)</p>
<p><br>
</p>
<div>
<div>
<blockquote type="cite" class="clean_bq"
style="color: rgb(0, 0, 0); font-family:
Helvetica, Arial; font-size: 13px;
font-style: normal; font-variant:
normal; font-weight: normal;
letter-spacing: normal; line-height:
normal; orphans: auto; text-align:
start; text-indent: 0px; text-transform:
none; white-space: normal; widows: auto;
word-spacing: 0px;
-webkit-text-stroke-width: 0px;">
<div bgcolor="#FFFFFF" text="#000000">
<div><span><br>
I realize that the assert on line
893 might have been useful if you
were playing around with the
numbers when you were
experimenting
with the patch. But now it looks
mostly like line noise to me. Same
with the assert on line 898.</span></div>
</div>
</blockquote>
</div>
<p><br>
</p>
<p>Actually, it as basically stating the
invariant for those
parameters (in case someone wants to play
around with them). But if
we make the constants into cmd line args
this will go away...</p>
<p><br>
</p>
<div>
<div>
<blockquote type="cite" class="clean_bq"
style="color: rgb(0, 0, 0);
font-family: Helvetica, Arial;
font-size: 13px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing:
0px; -webkit-text-stroke-width: 0px;">
<div bgcolor="#FFFFFF" text="#000000">
<div><span><br>
I would also consider factoring
the actual computation out into
a
separate helper method.</span></div>
</div>
</blockquote>
</div>
<p><br>
</p>
<p>You mean the interpolation? Like:</p>
<p>(base_min, base_max, value_min,
value_max, value) -> result
in range [base_min, base_max]?</p>
</div>
</div>
</div>
</div>
</blockquote>
<br>
Yes, that's what I meant. But if you, as suggested
above, get rid
of most (all?) of the constant definitions in
adjust_cards_per_stride() then I guess it is mostly
only the
interpolation left. So, in that case it may not be
necessary to
factor it out. I would have to see a new webrev to
have more
comments.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<blockquote
cite="mid:etPan.5582cc8d.1bcb7375.1215a@tw-mbp-tprintezis"
type="cite">
<div>
<div>
<div>
<div>
<p>Tony</p>
<p><br>
</p>
<div>
<blockquote type="cite" class="clean_bq"
style="color: rgb(0, 0, 0);
font-family: Helvetica, Arial;
font-size: 13px; font-style: normal;
font-variant: normal; font-weight:
normal; letter-spacing: normal;
line-height: normal; orphans: auto;
text-align: start; text-indent: 0px;
text-transform: none; white-space:
normal; widows: auto; word-spacing:
0px; -webkit-text-stroke-width: 0px;">
<div bgcolor="#FFFFFF" text="#000000">
<div><span><br>
I also second Jon's comment on
that it would be good with a
more
explicit way of turning this
feature off.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
</span>
<blockquote
cite="mid:etPan.5581f51b.699a1ea0.1215a@tw-mbp-tprintezis"
type="cite">
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span>(BTW, for some reason
some of the webrev output is
a bit
messed up. Not sure why,
maybe some hg
incompatibility I guess. The
diffs look OK though. I also
attached the patch to this
e-mail.)</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span>There’s a bit of info on
the JIRA on the rationale
for the
patch:</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span><a
moz-do-not-send="true"
href="https://bugs.openjdk.java.net/browse/JDK-8086056">https://bugs.openjdk.java.net/browse/JDK-8086056</a></span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span>The min / max values for
the old gen capacity
and ParGCCardsPerStrideChunk
were chosen empircally after
running a few (mostly
synthetic tests) on Linux
x64. If someone has
the cycles to do a more
extensive performance study,
I’d be happy
to revise them accordingly.</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span>Regards,</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span><br>
</span></div>
<div id="bloop_customfont"
style="font-family: Helvetica,
Arial; font-size: 13px; color:
rgb(0, 0, 0); margin: 0px;">
<span>Tony</span></div>
<span><br>
</span>
<div
id="bloop_sign_1434579909530055936"
class="bloop_sign">
<div style="font-family:
helvetica, arial; font-size:
13px;">
<div><span>-----</span></div>
<div><span><br>
</span></div>
<div><span>Tony Printezis |
JVM/GC Engineer / VM
Team |
Twitter</span></div>
<div><span><br>
</span></div>
<div><span>@TonyPrintezis</span></div>
<div><span><a
moz-do-not-send="true"
href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a></span></div>
<div><span><br>
</span></div>
</div>
</div>
</blockquote>
<span><br>
</span></div>
</div>
</blockquote>
<br class="Apple-interchange-newline">
</div>
<br class="Apple-interchange-newline">
</div>
<br class="Apple-interchange-newline">
</div>
<br class="Apple-interchange-newline">
</div>
<br class="Apple-interchange-newline">
</div>
<div id="bloop_sign_1434634117464453888"
class="bloop_sign">
<div
style="font-family:helvetica,arial;font-size:13px">
<div>-----</div>
<div><br>
</div>
<div>Tony Printezis | JVM/GC Engineer / VM
Team | Twitter</div>
<div><br>
</div>
<div>@TonyPrintezis</div>
<div><a moz-do-not-send="true"
href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a></div>
<div><br>
</div>
</div>
</div>
</blockquote>
<br>
</div>
</div>
</blockquote>
<div id="bloop_sign_1435076298751951872"
class="bloop_sign">
<div style="font-family:helvetica,arial;font-size:13px">
<div>-----</div>
<div><br>
</div>
<div>Tony Printezis | JVM/GC Engineer / VM Team |
Twitter</div>
<div><br>
</div>
<div>@TonyPrintezis</div>
<div><a moz-do-not-send="true"
href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a></div>
<div><br>
</div>
</div>
</div>
</div>
</div>
</span></blockquote>
<div id="bloop_sign_1435171154189988096" class="bloop_sign">
<div style="font-family:helvetica,arial;font-size:13px">
<div>-----</div>
<div><br>
</div>
<div>Tony Printezis | JVM/GC Engineer / VM Team | Twitter</div>
<div><br>
</div>
<div>@TonyPrintezis</div>
<div><a moz-do-not-send="true"
href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a></div>
<div><br>
</div>
</div>
</div>
</blockquote>
<br>
</body>
</html>