<div dir="ltr">Dear Tony,<div><br></div><div>I updated the webrev in-place with your suggestions.</div><div><br></div><div>Thank you for the review,</div><div>Carsten</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 18, 2016 at 11:01 AM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">Carsten,</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">Looks good.</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">In unlink_scavenge_root_nmethod() I’d just assert !UseG1GC, instead of doing if (UseG1GC) return;</div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">in the comments for scavenge_root_nmethods_do()  and CodeBlobToOopClosure: maybe f->fix_relocations -> f->fix_relocations() (3x)</div><span class="HOEnZb"><font color="#888888"><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto"><br></div><div style="font-family:Helvetica,Arial;font-size:13px;color:rgba(0,0,0,1.0);margin:0px;line-height:auto">Tony</div></font></span><div><div class="h5"> <br><p>On February 18, 2016 at 10:00:47 AM, Carsten Varming (<a href="mailto:varming@gmail.com" target="_blank">varming@gmail.com</a>) wrote:</p> <blockquote type="cite"><span><div><div></div><div>





<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/" target="_blank">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><br></span>
<p><span>On February 17, 2016 at 11:58:57 AM, Tony
Printezis (<a href="mailto:tprintezis@twitter.com" target="_blank">tprintezis@twitter.com</a>) wrote:</span></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">
<div style="word-wrap:break-word">
<div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span>Carsten,</span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span>I think this looks OK. A few
suggestions:</span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span>codeCache.{hpp,cpp}:</span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span>(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></span></div>
</div>
</div>
</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><span><font color="#888888">Tony</font></span></p>
<div>
<div>
<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">
<div style="word-wrap:break-word">
<div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span>Or maybe CodeBlobToOopClosure is a good place for this (as
that’s the closure that calls fix_oop_relocations()).</span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span>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?</span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span>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().</span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span><br></span></div>
<div style="font-family:Helvetica,Arial;font-size:13px;color:rgb(0,0,0);margin:0px">
<span>Tony</span></div>
<p><span>On February 16, 2016 at 9:54:56 PM, Carsten Varming
(<a href="mailto:varming@gmail.com" target="_blank">varming@gmail.com</a>) wrote:</span></p>
<blockquote type="cite">
<div>
<div>
<div dir="ltr"><span><span>Dear Jon,</span></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>
</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>


</div></div></span></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></div></blockquote></div><br></div>