<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;">One more observation inline.</div> <br><p class="airmail_on">On February 17, 2016 at 11:58:57 AM, Tony Printezis (<a href="mailto:tprintezis@twitter.com">tprintezis@twitter.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 style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div></div><div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">Carsten,</div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div id="bloop_customfont" 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 id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">codeCache.{hpp,cpp}:</div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div id="bloop_customfont" 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 class="Apple-converted-space"> </span></div></div></div></span></blockquote></div><p><br></p><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><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 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: 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 id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div id="bloop_customfont" 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 id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div id="bloop_customfont" 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 id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;"><br></div><div id="bloop_customfont" style="font-family: Helvetica, Arial; font-size: 13px; color: rgb(0, 0, 0); margin: 0px;">Tony</div><p class="airmail_on">On February 16, 2016 at 9:54:56 PM, Carsten Varming (<a href="mailto:varming@gmail.com">varming@gmail.com</a>) wrote:</p><blockquote type="cite" class="clean_bq"><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 class="Apple-converted-space"> </span><a href="http://cr.openjdk.java.net/~cvarming/1/webrev/">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 class="Apple-converted-space"> </span><span dir="ltr"><<a href="mailto:jon.masamitsu@oracle.com" target="_blank">jon.masamitsu@oracle.com</a>></span><span class="Apple-converted-space"> </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 class="HOEnZb"><font color="#888888"><br><br>Jon<br><br></font></span></font><div><div class="h5"><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 class="Apple-converted-space"> </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 id="bloop_sign_1455726579849223168" 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><br class="Apple-interchange-newline"></div> <div id="bloop_sign_1455731032669880832" 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>