<div dir="ltr"><div style="font-size:12.8000001907349px">(Resent from my gmail id: as my twitter email id isn't registered with the openjdk lists; please pardon duplicates.)</div><div style="font-size:12.8000001907349px"><br></div><div style="font-size:12.8000001907349px">Looks good to me too. Minor remarks regarding the documentation comments:</div><div style="font-size:12.8000001907349px"><br></div><div style="font-size:12.8000001907349px"><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><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></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 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 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></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><span class=""><font color="#888888"><pre style="white-space:pre-wrap"><font face="arial, helvetica, sans-serif">-- ramki</font></pre></font></span></pre></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 28, 2016 at 2:06 PM, Tony Printezis <span dir="ltr"><<a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">Thanks Jon! New webrev here:</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto"><br></div><div 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/8155257/webrev.1/" target="_blank">http://cr.openjdk.java.net/~tonyp/8155257/webrev.1/</a></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">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. </div><span class="HOEnZb"><font color="#888888"><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">Tony</div></font></span><div><div class="h5"> <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"><span><div bgcolor="#FFFFFF" text="#000000"><div></div><div>
<br>
<br>
<div>On 04/28/2016 01:51 PM, Tony Printezis
wrote:<br></div>
<blockquote type="cite">
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
Jon (CCing Ramki),</div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<br></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
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?</div>
</blockquote>
<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>
<blockquote type="cite">
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<br></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
Tony</div>
<br>
<p>On April 28, 2016 at 4:33:44 PM, 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:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<span>Hi Jon,</span></div>
<div 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 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! </span></div>
<div 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 style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<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:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<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:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<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:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<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:rgba(0,0,0,1.0);margin:0px;line-height:auto">
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:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<br></div>
<div 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/%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:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<br></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
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:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<br></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
I’ll run more testing overnight.</div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
<br></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">
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></span></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>