<div dir="ltr">Dear Tony and Jon,<div><br></div><div>I have updated the webrev (<a href="http://cr.openjdk.java.net/~cvarming/scavenge_nmethods_auto_prune/2/">http://cr.openjdk.java.net/~cvarming/scavenge_nmethods_auto_prune/2/</a>) with your suggestions.</div><div><br></div><div>Carsten<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Feb 17, 2016 at 12:45 PM, Tony Printezis <span dir="ltr"><<a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">One more observation inline.</div><span class=""> <br><p>On February 17, 2016 at 11:58:57 AM, Tony Printezis (<a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a>) wrote:</p> <div><blockquote type="cite" style="font-family:Helvetica,Arial;font-size:13px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span><div style="word-wrap:break-word"><div></div><div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">Carsten,</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">I think this looks OK. A few suggestions:</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">codeCache.{hpp,cpp}:</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">(Unrelated to your change) Since you’re at it, could you add a big warning to the declaration of scavenge_root_nmethods_do() that, if the caller is moving objects, it has to move them promptly when the closure is invoked and not enqueue them for later processing? All the uses of this are correct as far as I can see. For example, PS does push on the taskqueues oop*’s for later processing but in the closure that it passes to scavenge_root_nmethods_do(), it copies (in fact: it promotes) the objects immediately. If the copy is postponed, fix_oop_relocations() will pick up the wrong object(s). It’s a subtle point that I don’t think it’s properly documented. It’d be good to document it somewhere.<span> </span></div></div></div></span></blockquote></div><p><br></p></span><p>In fact, if the closure doesn’t copy an object immediately, detect_scavenge_root_oops() could return the wrong answer. So, it does also affect the logic in scavenge_root_nmethods_do(). (FWIW)</p><span class=""><font color="#888888"><p>Tony</p></font></span><div><div class="h5"><p><br></p><div><blockquote type="cite" style="font-family:Helvetica,Arial;font-size:13px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span><div style="word-wrap:break-word"><div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">Or maybe CodeBlobToOopClosure is a good place for this (as that’s the closure that calls fix_oop_relocations()).</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">Given that scavenge_root_nmethods() now prunes the list, could you add a comment to the declaration of prune_scavenge_root_nmethods() to indicate where it is used (i.e., to prune the list post full compaction I guess?). In fact, is prune_scavenge_root_nmethods() called from outside the CodeCache now? If not, maybe make it private?</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">The semantics of drop_scavenge_root_nmethod(), the way you changed it, are a bit confused. prev == NULL might mean “we don’t know the prev of nm, look for it” or “nm is the head”. I’d suggest leave drop_scavenge_root_nmethod() as is (semantic-wise), introduce a new (private) method (like: unlink_scavenge_root_nmethod(nm, prev)) which does the unlinking, and call it from drop_scavenge_root_nmethod() and scavenge_root_nmethods_do().</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">Tony</div><p>On February 16, 2016 at 9:54:56 PM, Carsten Varming (<a href="mailto:varming@gmail.com" target="_blank">varming@gmail.com</a>) wrote:</p><blockquote type="cite"><div><div><div dir="ltr"><span>Dear Jon,</span><div><span><br></span></div><div><span>Oops, I don't know what I was thinking when I found that ticket. I'll try to put it back in its old state. I filed a new ticket: JDK-8150013.</span></div><div><span><br></span></div><div><span>I folded the pruning of the scavengable nmethod list into CodeCache::scavenge_root_nmethods_do, thus removing the need for a separate pruning step after the young collection.</span></div><div><span><br></span></div><div><span>You can find the new webrev here:<span> </span><a href="http://cr.openjdk.java.net/~cvarming/1/webrev/" target="_blank">http://cr.openjdk.java.net/~cvarming/1/webrev/</a></span></div><div><span><br></span></div><div><span>Carsten</span></div><div><span><br></span></div><div><span><br></span></div></div><div class="gmail_extra"><span><br></span><div class="gmail_quote"><span>On Tue, Feb 16, 2016 at 6:53 PM, Jon Masamitsu<span> </span><span dir="ltr"><<a href="mailto:jon.masamitsu@oracle.com" target="_blank">jon.masamitsu@oracle.com</a>></span><span> </span>wrote:<br></span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div text="#000000" bgcolor="#FFFFFF"><font face="Times New Roman, Times, serif">Carsten,<br><br>This looks like a good change, but 8080232 describes a different problem.<br>8080232 proposed early promotion for objects in nmethods.  This change<br>removes nmethods from a list when it is determined that they don't refer to<br>scavengable objects.  Can you create a new CR for this?<span><font color="#888888"><br><br>Jon<br><br></font></span></font><div><div><br><div>On 02/15/2016 12:53 PM, Carsten Varming wrote:<br></div><blockquote type="cite"><div dir="ltr">Dear GC members,<div><br></div><div>I would like to contribute a fix for <a href="http://JDK-8080232" target="_blank">JDK-8080232</a>. The bug is marked as resolved because it was withdrawn. The fix is simple, so I would like to reopen the ticket. At Twitter we have seen the time to fix relocations after a ParNew cycle drop by three to four orders of magnitude with this patch.</div><div><br></div><div>I have put a webrev here:<span> </span><a href="http://cr.openjdk.java.net/%7Ecvarming/prune_scavengable_nmethods/0/webrev/" target="_blank">http://cr.openjdk.java.net/~cvarming/prune_scavengable_nmethods/0/webrev/</a></div><div><br></div><div>I will need a sponsor.</div><div><br></div><div>Carsten</div></div></blockquote><br></div></div></div></blockquote></div><br></div></div></div></blockquote><div><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" target="_blank">tprintezis@twitter.com</a></div><div><br></div></div></div></div></div></span></blockquote><br></div> <div><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" target="_blank">tprintezis@twitter.com</a></div><div><br></div></div></div></div></div></div></blockquote></div><br></div></div></div>