
<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;">Thanks for looking at it. See inline...</div> <br><p class="airmail_on">On March 3, 2016 at 9:35:26 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="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 Tue, 2016-03-01 at 13:22 -0500, Tony Printezis wrote:<span class="Apple-converted-space"> </span><br>> Hi all,<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> These are the per-worker preserved mark stack changes for ParallelGC:<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> http://cr.openjdk.java.net/~tonyp/8146991/webrev.0/<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> They are similar to the ParNew changes and rely on some classes I<span class="Apple-converted-space"> </span><br>> introduced then.<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br><br>- In PSPromotionManager::initialize(), please extract the<span class="Apple-converted-space"> </span><br>"ParallelGCThreads + 1" calculation into a local variable.<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="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>- nitpick: the comment in PSPromotionManager::post_scavenge(), since it is a proper sentence, start with upper case and end with full stop.<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="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>- in PreservedMarksSet::restore(), I would prefer if the total_size<span class="Apple-converted-space"> </span><br>were calculated as the marks are restored. I do not see a particular<span class="Apple-converted-space"> </span><br>reason why you would need to calculate this beforehand, and it is an<span class="Apple-converted-space"> </span><br>extra loop over a potentially large number of threads.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>I can accumulate the size incrementally, so the output should be done at the end of the method then?</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="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>Like PreservedMarks::get() returned the number of preserved marks, and<span class="Apple-converted-space"> </span><br>the method just added them together.<span class="Apple-converted-space"> </span><br><br>- I personally dislike the use of "counter-variable += 1" in the<span class="Apple-converted-space"> </span><br>increment section of for-loops, and prefer using a post-decrement. Feel<span class="Apple-converted-space"> </span><br>free to ignore this comment.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>I’ve always done it this way, ++i / i++ as a statement just looks weird...</p><p><br></p><div><div><blockquote type="cite" class="clean_bq" style="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>> Quick question on this: I moved:<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> log_trace(gc, ergo)("Restoring " SIZE_FORMAT " marks", total_size());<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> to the shared code so that the message is logged irrespective of GC<span class="Apple-converted-space"> </span><br>> (for example, I don’t think we were logging this for ParNew or<span class="Apple-converted-space"> </span><br>> DefNew). Is “ergo” the right category here though?<span class="Apple-converted-space"> </span><br><br>- I would remove the "ergo" tag, and try to make the message more<span class="Apple-converted-space"> </span><br>meaningful, as the "gc" tag is the catch-all bucket. E.g. something<span class="Apple-converted-space"> </span><br>like "Restoring... marks after evacuation failure.".<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Done. I’ll post a new webrev shortly…</p><p>Tony</p><p><br></p><div><blockquote type="cite" class="clean_bq" style="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>Otherwise it looks good to me. I will run it through jprt and some test<span class="Apple-converted-space"> </span><br>list.<span class="Apple-converted-space"> </span><br><br>Thanks,<span class="Apple-converted-space"> </span><br>Thomas<span class="Apple-converted-space"> </span><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> <div id="bloop_sign_1457049249104833024" 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>