
<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;">Inline.</div> <br><p class="airmail_on">On March 22, 2016 at 7:01:32 AM, Thomas Schatzl (<a href="mailto:thomas.schatzl@oracle.com">thomas.schatzl@oracle.com</a>) wrote:</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;"><span><div><div></div><div>Hi Tony,<span class="Apple-converted-space"> </span><br><br>some comments to your questions:<span class="Apple-converted-space"> </span><br><br>On Thu, 2016-03-10 at 12:56 -0500, Tony Printezis wrote:<span class="Apple-converted-space"> </span><br>> Change to use the newly-introduced PreservedMarks* classes for<span class="Apple-converted-space"> </span><br>> preserving marks in G1:<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> http://cr.openjdk.java.net/~tonyp/8151556/webrev.0/<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> G1 already had per-worker mark stacks so this change was mostly<span class="Apple-converted-space"> </span><br>> straightforward. A few comments / questions:<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> - The main change to the PreservedMarks* classes is to add the<span class="Apple-converted-space"> </span><br>> par_restore() method so that G1 continues doing that phase in<span class="Apple-converted-space"> </span><br>> parallel. I changed the PreservedMarks::restore() method to do what<span class="Apple-converted-space"> </span><br>> G1 was doing (i.e., keep popping entries from the stack until the<span class="Apple-converted-space"> </span><br>> stack is empty) instead of iterating over the stack, then clearing<span class="Apple-converted-space"> </span><br>> it. The latter was, for some reason, faster in the serial case (what<span class="Apple-converted-space"> </span><br>> we’ve had so far in PreservedMarks). However, it doesn’t seem to<span class="Apple-converted-space"> </span><br>> parallelize well.<span class="Apple-converted-space"> </span><br><br>Contention in free() when everyone is finished?<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>I think so. Interleaving free with mark restoration seems to help a bit. FWIW, we also increased the stack segment size to 64K and that also improves things further...</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;"><span><div><div><br>Some experiments in this area (with G1) showed that the work<span class="Apple-converted-space"> </span><br>distribution is far from optimal btw, particularly for smaller sets of<span class="Apple-converted-space"> </span><br>marks to restore.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Yes. BTW, given that the stacks are already chunked it might be relatively easy to take advantage of that to achieve better load balancing. Not sure it’s worth spending more time on this, though….</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;"><span><div><div><br>> So, I opted to copy what G1 was doing so that at least there’s no<span class="Apple-converted-space"> </span><br>> regression in G1. I also changed the logic slightly: the parallel<span class="Apple-converted-space"> </span><br>> workers now claim tasks to do (using the SequentialSubTasksDone<span class="Apple-converted-space"> </span><br>> class) instead of only doing the one that corresponds to their worker<span class="Apple-converted-space"> </span><br>> id (what G1 was doing before). This doesn’t seem to penalize this<span class="Apple-converted-space"> </span><br>> phase (at least in the tests I ran) and it’s a bit safer (if one<span class="Apple-converted-space"> </span><br>> worker wakes up late, maybe another one will do the work).<span class="Apple-converted-space"> </span><br><br>If a worker is later, this is still a problem, because if it is late it<span class="Apple-converted-space"> </span><br>is often very late. And at the end of that parallel phase they need to<span class="Apple-converted-space"> </span><br>synchronize anyway.<span class="Apple-converted-space"> </span></div></div></span></blockquote></div><p><br></p><p>Of course. But, if it eventually starts up late at least all the work will be done and the phase can complete immediately (instead of waiting even longer). No, if the worker takes 1min to start up, this of course won’t really help. :-) But in the common case, it might.</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;"><span><div><div><br>> - Changed the max cache size of the stacks to 0 so that the stacks do<span class="Apple-converted-space"> </span><br>> not cache any segments. Given that we don’t expect promotion /<span class="Apple-converted-space"> </span><br>> evacuation failures to happen very frequently, caching segments on<span class="Apple-converted-space"> </span><br>> these stacks is mostly wasted space. I also added asserts to ensure<span class="Apple-converted-space"> </span><br>> that there are no cached segments when the stacks are supposed to be<span class="Apple-converted-space"> </span><br>> empty.<span class="Apple-converted-space"> </span><br><br>Okay.<span class="Apple-converted-space"> </span><br><br>> - I wanted to have one method that has the logic to initialize an<span class="Apple-converted-space"> </span><br>> object’s mark word after promotion / evacuation failure. Both ParNew<span class="Apple-converted-space"> </span><br>> / ParallelGC use the RemoveForwardedPointerClosure, but G1 has its<span class="Apple-converted-space"> </span><br>> own closure (as it has to do some extra work). I introduced the<span class="Apple-converted-space"> </span><br>> init_forwarded_mark() method that’s called from both places. I could<span class="Apple-converted-space"> </span><br>> also make the G1 closure a subclass of RemoveForwardedPointerClosure<span class="Apple-converted-space"> </span><br>> and just call the do_object() on the parent. Or drop the idea of<span class="Apple-converted-space"> </span><br>> using the same method. Feedback welcome.<span class="Apple-converted-space"> </span><br><br>The current method looks fine to me.<span class="Apple-converted-space"> </span><br><br>><span class="Apple-converted-space"> </span><br>> - Now that G1 doesn’t use the OopAndMarkOop and OopAndMarkOopStack<span class="Apple-converted-space"> </span><br>> classes directly I made them private to PreservedMarks.<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> - Small re-arranging of the #include declarations in<span class="Apple-converted-space"> </span><br>> preservedMarks.inline.hpp as I had put them before the #ifndef guard<span class="Apple-converted-space"> </span><br>> for some reason (oops! sorry…)<span class="Apple-converted-space"> </span><br>><span class="Apple-converted-space"> </span><br>> - As it was straightforward, I also call par_restore() from ParNew<span class="Apple-converted-space"> </span><br>> too. I opted to decide whether to call the serial or parallel cases<span class="Apple-converted-space"> </span><br>> in DefNew based on whether workers() is NULL or not. I can also do<span class="Apple-converted-space"> </span><br>> that with a virtual method on DefNew which is overwritten in ParNew.<span class="Apple-converted-space"> </span><br>> Your call. BTW, I can do this change on a separate CR too if it’s<span class="Apple-converted-space"> </span><br>> considered outside the scope of this one (but it was pretty trivial<span class="Apple-converted-space"> </span><br>> so I think it’s OK piggy-backing it here).<span class="Apple-converted-space"> </span><br><br>I think the suggestion in the review thread for the extracted parallel<span class="Apple-converted-space"> </span><br>header restoration is best in terms of size of interface, lines of code<span class="Apple-converted-space"> </span><br>and code reuse opportunities. I.e. there does not seem to be a need to<span class="Apple-converted-space"> </span><br>use inheritance here imho.<span class="Apple-converted-space"> </span><br><br>Thanks,<span class="Apple-converted-space"> </span><br>Thomas<span class="Apple-converted-space"> </span><br></div></div></span></blockquote><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div><br class="Apple-interchange-newline"></div> <div id="bloop_sign_1458856992852333056" 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>