<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="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"><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;">
Change to use the newly-introduced PreservedMarks* classes for
preserving marks in G1:</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/8151556/webrev.0/">http://cr.openjdk.java.net/~tonyp/8151556/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;">
G1 already had per-worker mark stacks so this change was mostly
straightforward. A few comments / questions:</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;">
- 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).</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;">
- 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.</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 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.</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;">
- Now that G1 doesn’t use the OopAndMarkOop and OopAndMarkOopStack
classes directly I made them private to PreservedMarks.</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;">
<div id="bloop_customfont" style="margin: 0px;">- 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…)</div>
<div><br></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;">
- 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).</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,</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_1457630360430761984" 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_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></body></html>