<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;">Hi 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="margin: 0px;">First, thanks to Thomas for pushing the patch for <font face="Helvetica">8152312. Here’s a new webrev with the refactored G1 changes:</font></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 style="font-family: Helvetica;"><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;"><a href="http://cr.openjdk.java.net/~tonyp/8151556/webrev.1/">http://cr.openjdk.java.net/~tonyp/8151556/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;">Related question: Does G1 report on the GC log that an evacuation failure has occurred, the same way all the other GCs report promotion failures?</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;"><p style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;"><span style="font-variant-ligatures: no-common-ligatures">[6.319s][info][gc,promotion ] Promotion failed</span></p></div> <div><br></div>Should I add an extra message as part of this change?<div><br></div><div>Tony<br><p class="airmail_on">On March 21, 2016 at 12:02:02 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="color: rgb(0, 0, 0); margin: 0px;"><font face="Helvetica">I just
posted a separate code review request for:</font></div>
<div id="bloop_customfont" style="color: rgb(0, 0, 0); margin: 0px;"><font face="Helvetica"><br></font></div>
<div id="bloop_customfont" style="margin: 0px;">
<p style="color: rgb(0, 0, 0); margin: 0px;"><font face="Helvetica">8152312: ParNew: Restore preserved marks in
parallel</font></p>
<p style="color: rgb(0, 0, 0); margin: 0px;"><font face="Helvetica"><br></font></p>
<p style="margin: 0px;"><font face="Helvetica">which includes a
subset of the changes I originally posted here. Let’s freeze this
code review request (i.e., </font>8151556)<font face="Helvetica"> until the other one is done.</font></p>
<p style="margin: 0px;"><span style="font-family: Helvetica;"><br></span></p>
<p style="margin: 0px;"><span style="font-family: Helvetica;">Tony</span></p>
</div>
<br>
<p class="airmail_on">On March 10, 2016 at 12:56:11 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>Change to use the newly-introduced PreservedMarks* classes
for preserving marks in G1:</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><a href="http://cr.openjdk.java.net/~tonyp/8151556/webrev.0/">http://cr.openjdk.java.net/~tonyp/8151556/webrev.0/</a></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>G1 already had per-worker mark stacks so this change was
mostly straightforward. A few comments / questions:</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>- The main change to the PreservedMarks* classes is to add
the par_restore() method so that G1 continues doing that phase in
parallel. I changed the PreservedMarks::restore() method to do what
G1 was doing (i.e., keep popping entries from the stack until the
stack is empty) instead of iterating over the stack, then clearing
it. The latter was, for some reason, faster in the serial case
(what we’ve had so far in PreservedMarks). However, it doesn’t seem
to parallelize well. So, I opted to copy what G1 was doing so that
at least there’s no regression in G1. I also changed the logic
slightly: the parallel workers now claim tasks to do (using the
SequentialSubTasksDone class) instead of only doing the one that
corresponds to their worker id (what G1 was doing before). This
doesn’t seem to penalize this phase (at least in the tests I ran)
and it’s a bit safer (if one worker wakes up late, maybe another
one will do the work).</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>- Changed the max cache size of the stacks to 0 so that the
stacks do not cache any segments. Given that we don’t expect
promotion / evacuation failures to happen very frequently, caching
segments on these stacks is mostly wasted space. I also added
asserts to ensure that there are no cached segments when the stacks
are supposed to be empty.</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>- I wanted to have one method that has the logic to
initialize an object’s mark word after promotion / evacuation
failure. Both ParNew / ParallelGC use the
RemoveForwardedPointerClosure, but G1 has its own closure (as it
has to do some extra work). I introduced the init_forwarded_mark()
method that’s called from both places. I could also make the G1
closure a subclass of RemoveForwardedPointerClosure and just call
the do_object() on the parent. Or drop the idea of using the same
method. Feedback welcome.</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>- Now that G1 doesn’t use the OopAndMarkOop and
OopAndMarkOopStack classes directly I made them private to
PreservedMarks.</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;">
<div id="bloop_customfont" style="margin: 0px;"><span>- Small
re-arranging of the #include declarations in
preservedMarks.inline.hpp as I had put them before the #ifndef
guard for some reason (oops! sorry…)</span></div>
<div><span><br></span></div>
</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>- As it was straightforward, I also call par_restore() from
ParNew too. I opted to decide whether to call the serial or
parallel cases in DefNew based on whether workers() is NULL or not.
I can also do that with a virtual method on DefNew which is
overwritten in ParNew. Your call. BTW, I can do this change on a
separate CR too if it’s considered outside the scope of this one
(but it was pretty trivial so I think it’s OK piggy-backing it
here).</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,</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>Tony</span></div>
<span><br></span>
<div id="bloop_sign_1457630360430761984" class="bloop_sign">
<div style="font-family:helvetica,arial;font-size:13px">
<div><span>-----</span></div>
<div><span><br></span></div>
<div><span>Tony Printezis | JVM/GC Engineer / VM Team |
Twitter</span></div>
<div><span><br></span></div>
<div><span>@TonyPrintezis</span></div>
<div><span><a href="mailto:tprintezis@twitter.com">tprintezis@twitter.com</a></span></div>
<div><span><br></span></div>
</div>
</div>
</div>
</div>
</blockquote>
<div id="bloop_sign_1458575935033316096" 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_1460552461082766080" 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></body></html>