<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
<div class="moz-cite-prefix">On 04/28/2016 01:51 PM, Tony Printezis
wrote:<br>
</div>
<blockquote cite="mid:etPan.572277d7.1c1f60f0.367@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;">Jon (CCing
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;">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 cite="mid:etPan.572277d7.1c1f60f0.367@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;">Tony</div>
<br>
<p class="airmail_on">On April 28, 2016 at 4:33:44 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 Jon,</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! </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;">
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.)</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;">
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.</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 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?</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">On April 28, 2016 at 1:20:55 PM, Jon
Masamitsu (<a moz-do-not-send="true"
href="mailto:jon.masamitsu@oracle.com">jon.masamitsu@oracle.com</a>)
wrote:</p>
<blockquote type="cite" class="clean_bq">
<div bgcolor="#FFFFFF" text="#000000">
<div><span><font face="Times New Roman, Times, serif">Tony,<br>
<br>
Changes look good. Couple of small points.<br>
<br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etonyp/8155257/webrev.0/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp.frames.html">http://cr.openjdk.java.net/~tonyp/8155257/webrev.0/src/share/vm/gc/cms/concurrentMarkSweepGeneration.cpp.frames.html</a><br>
</font><br>
</span>
<pre><span><span class="new">1097 // This method should be called at the end of the main ParNew</span>
<span class="new">1098 // parallel phase to collapse the promoted object lists. Given that</span>
<span class="new">1099 // we don't want promoted objects to be tracked in future phases</span>
<span class="new">1100 // (e.g., during reference processing) we also disable promote</span>
<span class="new">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 class="new">2133 // I don't think this is really needed, as promotion tracking should</span>
<span class="new">2134 // have already been disabled. However, it sanity checks that the</span>
<span class="new">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 class="moz-cite-prefix">On 04/27/2016 01:40 PM,
Tony Printezis
wrote:<br>
</div>
<blockquote
cite="mid:etPan.572123ac.3113880d.19a@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;">
Small changes to clean up when promoted object
tracking is enabled
/ disabled and when tearing down the promoted
object lists is
done:</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"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Etonyp/8155257/webrev.0/">http://cr.openjdk.java.net/~tonyp/8155257/webrev.0/</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;">
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 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 run more testing overnight.</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>
<div id="bloop_sign_1461789445951004928"
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_1461874282101289984"
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_1461876447391315968" 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>