<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;">Sorry, just realized I made a mistake:</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;"><pre> 64 void PreservedMarksSet::restore() {
<span class="new" style="color: blue; font-weight: bold;"> 65 size_t total_size = 0;</span>
66 for (uint i = 0; i < _num; i += 1) {
67 get(i)->restore();
<span class="new" style="color: blue; font-weight: bold;"> 68 total_size += get(i)->size();</span>
69 }</pre></div> <div>I should of course add the size before restore() is called (as it will be 0 afterwards). I’ll fix that tomorrow. Let me know if there’s anything else I should change...</div><div><br></div><div>Tony</div><br><p class="airmail_on">On March 3, 2016 at 7:15:13 PM, Tony Printezis (<a 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></div><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;">
Thomas (and all),</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;">
Updated webrev with the changes below:</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/8146991/webrev.1/">http://cr.openjdk.java.net/~tonyp/8146991/webrev.1/</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;">
Tony</div>
<br>
<p class="airmail_on">On March 3, 2016 at 7:02:03 PM, Tony
Printezis (<a href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a>)
wrote:</p>
<blockquote type="cite" class="clean_bq">
<div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">
<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;">
<span>Thomas.</span></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;">
<span><br></span></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;">
<span>Thanks for looking at it. See inline...</span></div>
<span><br></span>
<p class="airmail_on"><span>On March 3, 2016 at 9:35:26 AM, Thomas
Schatzl (<a href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>)
wrote:</span></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;">
<div>
<div><span><span>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></span></span></div>
</div>
</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;">
<div>
<div><span><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></span></div>
</div>
</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;">
<div>
<div><span><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></span></div>
</div>
</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;">
<div>
<div><span><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></span></div>
</div>
</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;">
<div>
<div><span><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></span></div>
</div>
</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;">
<div>
<div><span><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></span></div>
</div>
</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>
</div>
</div>
</blockquote>
<div id="bloop_sign_1457050487900008960" 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>
</div></div></span></blockquote> <div id="bloop_sign_1457054710684784896" 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>