RFR 8150013: ParNew: Prune nmethods scavengable list

[Carsten Varming]

Dear Tony and Jon,

I have updated the webrev (
http://cr.openjdk.java.net/~cvarming/scavenge_nmethods_auto_prune/2/) with
your suggestions.

Carsten

On Wed, Feb 17, 2016 at 12:45 PM, Tony Printezis <tprintezis at twitter.com>
wrote:

> One more observation inline.
>
> On February 17, 2016 at 11:58:57 AM, Tony Printezis (
> tprintezis at twitter.com) wrote:
>
> Carsten,
>
> I think this looks OK. A few suggestions:
>
> codeCache.{hpp,cpp}:
>
> (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.
>
>
> 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)
>
> Tony
>
>
> Or maybe CodeBlobToOopClosure is a good place for this (as that’s the
> closure that calls fix_oop_relocations()).
>
> 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?
>
> 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().
>
> Tony
>
> On February 16, 2016 at 9:54:56 PM, Carsten Varming (varming at gmail.com)
> wrote:
>
> Dear Jon,
>
> 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.
>
> 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.
>
> You can find the new webrev here:
> http://cr.openjdk.java.net/~cvarming/1/webrev/
>
> Carsten
>
>
>
> On Tue, Feb 16, 2016 at 6:53 PM, Jon Masamitsu <jon.masamitsu at oracle.com>
> wrote:
>
>> Carsten,
>>
>> This looks like a good change, but 8080232 describes a different problem.
>> 8080232 proposed early promotion for objects in nmethods.  This change
>> removes nmethods from a list when it is determined that they don't refer
>> to
>> scavengable objects.  Can you create a new CR for this?
>>
>> Jon
>>
>>
>> On 02/15/2016 12:53 PM, Carsten Varming wrote:
>>
>> Dear GC members,
>>
>> I would like to contribute a fix for JDK-8080232. 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.
>>
>> I have put a webrev here:
>> http://cr.openjdk.java.net/~cvarming/prune_scavengable_nmethods/0/webrev/
>>
>> I will need a sponsor.
>>
>> Carsten
>>
>>
>>
> -----
>
> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>
> @TonyPrintezis
> tprintezis at twitter.com
>
>
> -----
>
> Tony Printezis | JVM/GC Engineer / VM Team | Twitter
>
> @TonyPrintezis
> tprintezis at twitter.com
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160218/592fd4ee/attachment.htm>