<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;">Ramki,</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;">Inline.</div> <br><p class="airmail_on">On April 29, 2016 at 1:52:01 PM, Srinivas Ramakrishna (<a href="mailto:ysr1729@gmail.com">ysr1729@gmail.com</a>) wrote:</p> <div><blockquote type="cite" class="clean_bq" style="font-family: Helvetica, Arial; font-size: 13px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: 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><div dir="ltr">(responding from my gmail because of list membership, but please keep my twitter id in cc so I know when to look :-)<div><br></div><div>Haven't yet looked at yr webrev, but just wanted to send this quick remark .... (inline below) ....<br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 29, 2016 at 8:03 AM, Tony Printezis<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;"><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">Ramki and Jon,</div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">I hate to confused things further. :-) I just realized that if only ParNew needs the promotion tracking to be enabled for the parallel workers, aren’t we better off just letting ParNew enable / disable it when it needs to instead of always doing so in the CMSGeneration prologue / epilogue?</div></div></blockquote><div><br></div><div><br></div><div>.... Well, it's the non-contiguity of the CMS generation that requires promotion object tracking.</div><div>It doesn't matter if we have one worker thread or many.<span class="Apple-converted-space"> </span></div></div></div></div></div></div></div></span></blockquote></div><p><br></p><p>I understand. But please note that the CMS per-worker state which contains the per-worker PromotionInfo objects (note: CMS, not ParNew; i.e., the CMSParGCThreadState) is only used by ParNew as far as I can see. The serial code (which in this case is only used during serial ref processing, DefNew has been disabled for use with CMS in JDK 9) uses a separate PromotionInfo object (_promoInfo on CompactibleFreeListSpace) which is enabled / disabled separately (in CFLS::save_marks() and CFLS::gc_epilogue() resp.). </p><p><br></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-caps: normal; font-weight: normal; letter-spacing: 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 dir="ltr"><div><div class="gmail_extra"><div class="gmail_quote"><div>Since promotion can happen into</div><div>dirty cards which are being scanned "concurrently" (i.e. interleaved in the case of a single</div><div>worker), and our closures do not like to double-scan objects, we need to prevent that by</div><div>specially marking promoted objects while card scanning is in progress.</div><div><br></div><div>More after I have looked at yr webrev (much later this PM)...</div><div><br></div><div>-- ramki</div><div> </div><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;"><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">Here’s an alternative webrev for your consideration:</div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><a href="http://cr.openjdk.java.net/~tonyp/8155257/webrev.3.suggestion/" target="_blank">http://cr.openjdk.java.net/~tonyp/8155257/webrev.3.suggestion/</a></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">I think I prefer this approach but I’m OK with either. (And, yes, I’ll actually expand the comments I’ve marked as TODO if you prefer this one.)</div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span class="HOEnZb"><font color="#888888"><br></font></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span class="HOEnZb"><font color="#888888">Tony</font></span></div><div><div class="h5"><br><p>On April 29, 2016 at 10:13:27 AM, Tony Printezis (<a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a>) wrote:</p><blockquote type="cite"><div style="word-wrap: break-word;"><div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Ramki,</span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Thanks for the feedback. Latest webrev:</span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><a href="http://cr.openjdk.java.net/~tonyp/8155257/webrev.2/" target="_blank">http://cr.openjdk.java.net/~tonyp/8155257/webrev.2/</a></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>I rephrased the three comments in concurrentMarkSweepGeneration.cpp before the calls to {start,stop}TrackingPromotions(). The logic should be the same.</span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Tony</span></div><span><br></span><p><span>On April 28, 2016 at 9:48:26 PM, Srinivas Ramakrishna (<a href="mailto:ysr1729@gmail.com" target="_blank">ysr1729@gmail.com</a>) wrote:</span></p><blockquote type="cite"><div><div><div dir="ltr"><div style="font-size: 12.8000001907349px;"><span><span>(Resent from my gmail id: as my twitter email id isn't registered with the openjdk lists; please pardon duplicates.)</span></span></div><div style="font-size: 12.8000001907349px;"><span><br></span></div><div style="font-size: 12.8000001907349px;"><span>Looks good to me too. Minor remarks regarding the documentation comments:</span></div><div style="font-size: 12.8000001907349px;"><span><br></span></div><div style="font-size: 12.8000001907349px;"><pre style="white-space: pre-wrap; color: rgb(0, 0, 0);"><span><span style="color: blue; font-weight: bold;">1098 // The par_oop_since_save_marks_iterate_done() method should be</span>
<span style="color: blue; font-weight: bold;">1099 // called at the end of the main ParNew parallel phase to collapse</span>
<span style="color: blue; font-weight: bold;">1100 // the promoted object lists. Given that we don't want promoted</span>
<span style="color: blue; font-weight: bold;">1101 // objects to be tracked in future phases (e.g., during reference</span>
<span style="color: blue; font-weight: bold;">1102 // processing) we disable promoted object tracking.</span>
</span>
</pre><pre style="white-space: pre-wrap; color: rgb(0, 0, 0);"><span style="color: blue; font-weight: bold;"><br></span>
</pre><pre style="white-space: pre-wrap; color: rgb(0, 0, 0);"><font face="arial, helvetica, sans-serif">Perhaps just say:</font>
</pre><pre style="white-space: pre-wrap; color: rgb(0, 0, 0);"><font face="arial, helvetica, sans-serif">// Because card-scanning has been completed, further promotions, if any,</font>
</pre><pre style="white-space: pre-wrap; color: rgb(0, 0, 0);"><font face="arial, helvetica, sans-serif" style="font-size: 12.8000001907349px;">// e.g. during reference processing, </font><span style="font-size: 12.8000001907349px; font-family: arial, helvetica, sans-serif;">will not need to keep track of promoted objects.</span>
</pre><pre style="white-space: pre-wrap; color: rgb(0, 0, 0);"><span style="font-family: arial, helvetica, sans-serif;">Also instead of:</span>
</pre><pre style="white-space: pre-wrap;"></pre><pre style="white-space: pre-wrap; color: rgb(0, 0, 0);"><span style="color: blue; font-weight: bold;">// Enable promotion tracking for the main parallel ParNew phase.</span>
</pre><pre style="white-space: pre-wrap;"><font face="arial, helvetica, sans-serif">perhaps:</font>
</pre><pre style="white-space: pre-wrap;"><font face="arial, helvetica, sans-serif">// We track promoted objects to allow card-scanning to skip them.</font>
</pre><pre style="white-space: pre-wrap;"><font face="arial, helvetica, sans-serif">and instead of:</font>
</pre><pre style="white-space: pre-wrap;"></pre><pre style="white-space: pre-wrap;"><font face="arial, helvetica, sans-serif">But leaving them as is fine too.</font>
</pre><pre style="white-space: pre-wrap;"><font face="arial, helvetica, sans-serif">reviewed!</font>
</pre><pre style="white-space: pre-wrap;"><span><font color="#888888"><font face="arial, helvetica, sans-serif">-- ramki</font></font></span>
</pre><pre style="white-space: pre-wrap; color: rgb(0, 0, 0);"><span style="color: blue;">2133 // Promotion tracking should be disabled at the end of the ParNew</span>
<span style="color: blue;">2134 // parallel phase....</span>
</pre><pre style="white-space: pre-wrap;"><font color="#000000" face="arial, helvetica, sans-serif">perhaps:</font>
</pre><pre style="white-space: pre-wrap;"><font color="#000000" face="arial, helvetica, sans-serif">// When using ParNew, promoted object tracking would already have been disabled, however, ...</font>
</pre></div><div class="gmail_extra"><span><font color="#888888"><br></font></span><div class="gmail_quote"><span><font color="#888888">On Thu, Apr 28, 2016 at 2:06 PM, Tony Printezis<span class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a>></span><span class="Apple-converted-space"> </span>wrote:<br></font></span><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex;"><div style="word-wrap: break-word;"><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Thanks Jon! New webrev here:</span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><a href="http://cr.openjdk.java.net/~tonyp/8155257/webrev.1/" target="_blank">http://cr.openjdk.java.net/~tonyp/8155257/webrev.1/</a></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Compared to the previous one, I only changed the comments before the two calls to stopTrackingPromotions(). The logic should be the same. Will also run another set of tests overnight. </span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><font color="#888888"><br></font></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><font color="#888888">Tony</font></span></div><div><div><br><p>On April 28, 2016 at 5:02:17 PM, Jon Masamitsu (<a href="mailto:jon.masamitsu@oracle.com" target="_blank">jon.masamitsu@oracle.com</a>) wrote:</p><blockquote type="cite"><div bgcolor="#FFFFFF" text="#000000"><div><span><br><br></span><div><span>On 04/28/2016 01:51 PM, Tony Printezis wrote:<br></span></div><blockquote type="cite"><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Jon (CCing Ramki),</span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Re: comment 2: Actually, it looks as if we have to call stopPromotionTracking in the epilogue. The prologue / epilogue are called irrespective of the type of GC. So they are also called for Full GCs which won’t call the method to tear down the lists. I think it’d be safer to just call stopPromotionTracking() in the epilogue, as I had it before, with an amended comment?</span></div></blockquote><span><br>That sounds good. I like the symmetry of putting the stopPromotionTracking in the epilogue. <br><br>And yes, I'll run some addition tests when the latest patch comes out.<br><br>Jon<br><br></span><blockquote type="cite"><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Tony</span></div><span><br></span><p><span>On April 28, 2016 at 4:33:44 PM, Tony Printezis (<a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a>) wrote:</span></p><blockquote type="cite"><div style="word-wrap: break-word;"><div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><span>Hi Jon,</span></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Thanks for looking at it! </span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>comment 1 : “this method” refers to the enclosing method (i.e., par_oop_since_save_marks_iterate_done()). I’ll clarify. (I also spotted a typo in the comment! I’ll fix that too.)</span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>comment 2 : You’re right, actually. Good suggestion. I thought it might be awkward because of the ParNew-CMS interaction going through the generation abstractions. But in this code I’m already in the CMS generation and I can access the promoInfo directly. So I’ll just assert that tracking is off and the list is empty.</span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>I’ll post a new webrev in a bit. FWIW, overnight testing didn’t reveal any issues (and I’ll run it again given the “stop tracking” -> “sanity checks” change). Any chance you can also run some tests when I post the new webrev just in case?</span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span><br></span></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><span>Tony</span></div><span><br></span><p><span>On April 28, 2016 at 1:20:55 PM, Jon Masamitsu (<a href="mailto:jon.masamitsu@oracle.com" target="_blank">jon.masamitsu@oracle.com</a>) wrote:</span></p><blockquote type="cite"><div bgcolor="#FFFFFF" text="#000000"><div><span><span><font face="Times New Roman, Times, serif">Tony,<br><br>Changes look good. Couple of small points.<br><br><a href="http://cr.openjdk.java.net/%7Etonyp/8155257/webrev.0/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp.frames.html" target="_blank">http://cr.openjdk.java.net/~tonyp/8155257/webrev.0/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp.frames.html</a><br></font><br></span></span><pre><span><span>1097 // This method should be called at the end of the main ParNew</span>
<span>1098 // parallel phase to collapse the promoted object lists. Given that</span>
<span>1099 // we don't want promoted objects to be tracked in future phases</span>
<span>1100 // (e.g., during reference processing) we also disable promote</span>
<span>1101 // object tracking here.</span>
</span>
</pre><font face="Times New Roman, Times, serif">The placement of this block comment was slightly confusing. I wasn't<br>sure if "This method ..." applied specirfically to the stopTrackingPromotions()<br>until the last part "also disable ...". Would it be better placed before the method.<br></font><br><pre>2132 // Also reset promotion tracking in par gc thread states.
<span>2133 // I don't think this is really needed, as promotion tracking should</span>
<span>2134 // have already been disabled. However, it sanity checks that the</span>
<span>2135 // promotion lists are empty so I think it's helpful to leave it in.</span>
</pre>The comment makes clear you intent but is there a simpler and more direct<br>sanity check you can use? Comments sometimes get lost in the fog of code<br>churn and calling a method just for sanity checkinf seems wasteful and confusing<br>(Why is stopTrackingPromotions() call twice? Is there something that the<br>first all didn't do? Yes, you comment explains it but I have to read the comment.)<br>If you agree you can fix it at a later time. If you disagree, I'll silently forget about it :-).<br><br>Jon<br><br><div>On 04/27/2016 01:40 PM, Tony Printezis wrote:<br></div><blockquote type="cite"><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">Small changes to clean up when promoted object tracking is enabled / disabled and when tearing down the promoted object lists is done:</div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><a href="http://cr.openjdk.java.net/%7Etonyp/8155257/webrev.0/" target="_blank">http://cr.openjdk.java.net/~tonyp/8155257/webrev.0/</a></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">I also had to amend an assert which was too strong and removed the worker_id argument from the stopTrackingPromotions() method as it was actually not used.</div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">I’ll run more testing overnight.</div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">Tony</div><br><div><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" target="_blank">tprintezis@twitter.com</a></div><div><br></div></div></div></blockquote><br></div></div></blockquote><div><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" target="_blank">tprintezis@twitter.com</a></div><div><br></div></div></div></div></div></blockquote><div><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" target="_blank">tprintezis@twitter.com</a></div><div><br></div></div></div></blockquote><br></div></div></blockquote><div><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" target="_blank">tprintezis@twitter.com</a></div><div><br></div></div></div></div></div></div></blockquote></div><br></div></div></div></div></blockquote><div><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" target="_blank">tprintezis@twitter.com</a></div><div><br></div></div></div></div></div></blockquote><div><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" target="_blank">tprintezis@twitter.com</a></div><div><br></div></div></div></div></div></div></blockquote></div><br></div></div></div></div></div></span></blockquote><br class="Apple-interchange-newline"></div> <div id="bloop_sign_1461952392596084224" 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>