RFR 8150013: ParNew: Prune nmethods scavengable list
Jon Masamitsu
jon.masamitsu at oracle.com
Mon Mar 7 18:46:25 UTC 2016
Carsten,
Just to you.
You have author status so you could commit the patch in
mercurial and send me the patch for the changeset. That
would then list you as the user who submitted the change.
Or I could commit the patch and list you as the contributer.
Which would you like to do.
Jon
On 03/07/2016 09:02 AM, Jon Masamitsu wrote:
>
>
> 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