RFR 8150013: ParNew: Prune nmethods scavengable list

Mon Mar 7 01:51:21 UTC 2016

Dear Jon,

Thank you for sponsoring.

I am not sure about your intended change to the patch. If fix_relocations,
then there are two cases where I remove the nmethod from the scavengable
list: 1. If the nmethod is not live. 2. If the nmethod is live and it does
not have scavengable roots. With your suggested code change I don't think I
can cover both cases without code duplication. So I think it is best to
leave the logic as is.

Regarding you proposed comment about the precision of
detect_scavenge_root_oops.
// Unless the relocations have been updated (fixed), the
// detect_scavenge_root_oops() will not be precise so skip
// the check to unlink. Note that not doing the unlinking
// is safe but less optimal.

I don't know what you mean when you say "detect_scavenge_root_oops() will
not be precise". The method fix_relocations() in every closure that might
move an object returns true (I carefully checked all collectors except G1
(which does not use the list)), so there should never be a time where an
nmethod is left in a state where detect_scavenge_root_oops is wrong. You
are right that non-scavengable nmethods on the list is fine, but I disagree
about it being less than optimal (see next comment).

The reason I chose to not prune nmethods when !fix_relocations is that I
don't expect to be able to prune any live nmethods. To prune a live nmethod
from the list all oops in the nmethod must be non-scavengable
(oops::is_scavengable() must return false). In all collectors today,
fix_relocations is true if an object might be moved. Hence the only time
the list might contain a non-scavengable nmethod is between line 663 and
668 in my patch and between an oop moved by a full collection and the prune
method called at the end of said collection. As every live nmethod on the
list is expected to be scavengable (with the exceptions already listed)
pruning the list when objects have not moved seems like a waste. What about
dead nmethods? I don't think pruning dead nmethods really makes a
difference, but I would be happy to change the code to prune them
regardless of the value of fix_relocations.

I added the following comment

    // The scavengable nmethod list must contain all methods with
scavengable    // oops. It is safe to include more nmethod on the
list, but we do not    // expect any live non-scavengable nmethods on
the list.

I hope that covers your intention.

I updated the webrev at
http://cr.openjdk.java.net/~cvarming/scavenge_nmethods_auto_prune/2/ with
the comment.

Carsten

On Fri, Mar 4, 2016 at 1:52 PM, Jon Masamitsu <jon.masamitsu at oracle.com>
wrote:

> Carsten,
>
> There will be a couple of days delay in pushing this
> change.  The runtime repository is closed at the
> moment but hopefully will be open again in a day
> or two.
>
> Jon
>
>
> On 03/04/2016 10:23 AM, Carsten Varming wrote:
>
> Dear Christian,
>
> Thank you for your review.
>
> Carsten
>
> On Fri, Mar 4, 2016 at 12:50 PM, Christian Thalinger <
> <christian.thalinger at oracle.com>christian.thalinger at oracle.com> wrote:
>
>> Jon asked me to have a quick look over the code cache changes.  Looks
>> good to me.
>>
>> > On Mar 3, 2016, at 1:46 PM, Jon Masamitsu < <jon.masamitsu at oracle.com>
>> jon.masamitsu at oracle.com> wrote:
>> >
>> > Carsten,
>> >
>> > Changes look good.
>> >
>> > A couple of suggestions.
>> >
>> >
>> http://cr.openjdk.java.net/~cvarming/scavenge_nmethods_auto_prune/2/src/share/vm/code/codeCache.cpp.frames.html
>> >
>> > 661     if (is_live) {
>> > 662       // Perform cur->oops_do(f), maybe just once per nmethod.
>> > 663       f->do_code_blob(cur);
>> > 664     }
>> > 665 nmethod* const next = cur->scavenge_root_link();
>> > 666 if (fix_relocations) {
>> > 667 if (!is_live || !cur->detect_scavenge_root_oops()) {
>> > 668 unlink_scavenge_root_nmethod(cur, prev);
>> > 669 } else {
>> > 670 prev = cur;
>> > 671 }
>> > 672 }
>> > 673 cur = next;
>> >
>> > Although this does not change the actions, I find this easier to
>> > read.  You and Tony (and whoever else wants to) can vote
>> > on it.
>> >
>> > if (is_live) { ... } else {
>> > if (fix_relocations) { if (cur->detect_scavenge_root_oops()) { prev =
>> cur; } else { unlink_scavenge_root_nmethod(cur, prev);} } cur =
>> cur->scavenge_root_link(); }
>> >
>> > Also I'd suggest a comment before the if-test
>> > if you find it correct.
>> >
>> > // Unless the relocations have been updated (fixed), the //
>> detect_scavenge_root_oops() will not be precise so skip // the check to
>> unlink. Note that not doing the unlinking // is safe but less optimal. if
>> (fix_relocations) {
>> >
>> > The comment in the header file for fix_relocations helped
>> > but a comment here might be helpful.
>> >
>> > I'll sponsor.
>> >
>> > Jon
>> >
>> > On 2/28/2016 6:16 PM, Carsten Varming wrote:
>> >> Dear Hotspot developers,
>> >>
>> >> Any chance of a review of this patch? The patch cut between 7ms and
>> 10ms of every ParNew with one application at Twitter and I expect a 1-2ms
>> improvement for most applications.
>> >>
>> >> I touch the code cache and GenCollectedHeap, so I assume I need
>> reviews from both gc and compiler reviewers. Thank you Tony Printezis for
>> the review (posted on the hotspot-gc-dev list).
>> >>
>> >> I also need a sponsor.
>> >>
>> >> Carsten
>> >>
>> >> On Fri, Feb 19, 2016 at 10:52 AM, Carsten Varming <varming at gmail.com
>> <mailto:varming at gmail.com>> wrote:
>> >>
>> >>    Dear Hotspot developers,
>> >>
>> >>    I would like to contribute a patch for JDK-8150013
>> >>    <https://bugs.openjdk.java.net/browse/JDK-8150013>. The current
>> >>    webrev can be found here:
>> >>
>> http://cr.openjdk.java.net/~cvarming/scavenge_nmethods_auto_prune/2/
>> >>    <
>> http://cr.openjdk.java.net/%7Ecvarming/scavenge_nmethods_auto_prune/2/>.
>> >>
>> >>    Suggestions for improvements are very welcome.
>> >>
>> >>    Carsten
>> >>
>> >>
>> >
>>
>>
>
>