<html><head><style>body{font-family:Helvetica,Arial;font-size:13px}</style></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div id="bloop_customfont" style="font-family:Helvetica,Arial;font-size:13px; color: rgba(0,0,0,1.0); margin: 0px; line-height: auto;">Thomas,</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;">Latest version:</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 href="http://cr.openjdk.java.net/~tonyp/8086056/webrev.3/">http://cr.openjdk.java.net/~tonyp/8086056/webrev.3/</a></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;">See below for comments.</div> <br><p class="airmail_on" style="color:#000;">On July 2, 2015 at 4:43:42 AM, Thomas Schatzl (<a href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>) wrote:</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;"><span><div><div></div><div>Hi Tony,<span class="Apple-converted-space"> </span><br><br>On Wed, 2015-07-01 at 18:05 -0400, Tony Printezis wrote:<span class="Apple-converted-space"> </span><br>> Latest changes, as discussed with Bengt and Thomas,<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> http://cr.openjdk.java.net/~tonyp/8086056/webrev.2/<span class="Apple-converted-space"> </span><br><br>a few suggestions:<span class="Apple-converted-space"> </span><br><br>- please use braces even for simple statements in if-clauses, e.g.<span class="Apple-converted-space"> </span></div></div></span></blockquote></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;"><span><div><div><br>parNewGeneration.cpp:<span class="Apple-converted-space"> </span><br><br>883 if (!UseDynamicParGCStrides) return;<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>I did that. But I actually added an extra verbose statement in the body of the if-statement (I thought it’d be helpful to also print the value of ParGCCardsPerStrideChunk when the dynamic calculation is off). So, I don’t have a simple return any more, but I’ll keep that in mind in the future (was it anywhere else? I didn’t immediately spot another occurrence of that).</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;"><span><div><div><br>- Comments should start with upper case:<span class="Apple-converted-space"> </span><br><br>parNewGeneration.hpp:<span class="Apple-converted-space"> </span><br><br>354 // automatically calculate ParGCCardsPerStrideChunk based on the<span class="Apple-converted-space"> </span><br>old<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Done.</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;"><span><div><div><br>- parameter calculation in ParNewGeneration::adjust_cards_per_stride:<span class="Apple-converted-space"> </span><br>- DynamicParGCStridesMinSize should be > 0 (only if !<span class="Apple-converted-space"> </span><br>UseDynamicParGCStrides is enabled, not sure if that can be encoded<span class="Apple-converted-space"> </span><br>within the framework)<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>If !UseDynamicParGCStrides, then both sets of min / max parameters are not used. I think it’d make the constraint checking a pain having to sanity-check parameters whether a flag is on or off (unless they mean different things in either case, of course).</p><p>I added a range check to the min / max stride size args, added a lower bound of 1, and decided to add an artificial upper bound of 32K (I have tested with up to 8K). Is that reasonable? We can revisit that if we need to increse it. Note that you can still set ParGCCardsPerStrideChunk to something higher if required.</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;"><span><div><div><br>- the issues with the Dynamic*Capacity variables Sangheon mentioned,<span class="Apple-converted-space"> </span><br>although I think it is fine that they are equal, just that "r" needs to<span class="Apple-converted-space"> </span><br>be set correctly.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Yeah, fixed. (See previous e-mail.)</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;"><span><div><div><br>Just wanting to check back if it was intended that the code always<span class="Apple-converted-space"> </span><br>rounds down, achieving the maximum stride only at the end of the<span class="Apple-converted-space"> </span><br>interval. E.g.<span class="Apple-converted-space"> </span><br><br>912 const int stride_size_log = log2_long((jlong) stride_size);<span class="Apple-converted-space"> </span><br>913 res = (size_t) 1 << stride_size_log;<span class="Apple-converted-space"> </span><br><br>It's fine with me, just asking.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>I did the simplest calculation. I can revise it if you want. Also note that if capacity > max_capacity, then stride size will be set to the max stride size.</p><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;"><span><div><div><br>Thanks,<span class="Apple-converted-space"> </span><br>Thomas<span class="Apple-converted-space"> </span><br><br><br><br></div></div></span></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><br class="Apple-interchange-newline"></div> <div id="bloop_sign_1435865248348184064" 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 href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a></div><div><br></div></div></div></body></html>