RFR 8150013: ParNew: Prune nmethods scavengable list
Jon Masamitsu
jon.masamitsu at oracle.com
Mon Mar 7 17:02:15 UTC 2016
On 03/06/2016 05:51 PM, Carsten Varming wrote:
> 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.
Ok.
>
> 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 think you are correct so no need to make a change.
>
> 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.
Thanks for the added comment.
>
> I updated the webrev at
> http://cr.openjdk.java.net/~cvarming/scavenge_nmethods_auto_prune/2/
> <http://cr.openjdk.java.net/%7Ecvarming/scavenge_nmethods_auto_prune/2/>
> with the comment.
I'll push this version.
Jon
>
> Carsten
>
> On Fri, Mar 4, 2016 at 1:52 PM, Jon Masamitsu
> <jon.masamitsu at oracle.com <mailto: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
>> <mailto: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 <mailto: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
>> <http://cr.openjdk.java.net/%7Ecvarming/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> <mailto: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/>
>> >>
>> <http://cr.openjdk.java.net/%7Ecvarming/scavenge_nmethods_auto_prune/2/>.
>> >>
>> >> Suggestions for improvements are very welcome.
>> >>
>> >> Carsten
>> >>
>> >>
>> >
>>
>>
>
>
More information about the hotspot-dev
mailing list