RFR 8150013: ParNew: Prune nmethods scavengable list

[Carsten Varming]

Dear Tony,

I updated the webrev in-place with your suggestions.

Thank you for the review,
Carsten

On Thu, Feb 18, 2016 at 11:01 AM, Tony Printezis <tprintezis at twitter.com>
wrote:

> Carsten,
>
> Looks good.
>
> In unlink_scavenge_root_nmethod() I’d just assert !UseG1GC, instead of
> doing if (UseG1GC) return;
>
> in the comments for scavenge_root_nmethods_do()  and CodeBlobToOopClosure:
> maybe f->fix_relocations -> f->fix_relocations() (3x)
>
> Tony
>
> On February 18, 2016 at 10:00:47 AM, Carsten Varming (varming at gmail.com)
> wrote:
>
> 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
>>
>>
> -----
>
> 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/0b6dfdf4/attachment.htm>