<div dir="ltr">Hi All, <div><br></div><div>Let's revive the thread as it seems Bengt's change made in.</div><div>So before I upload the webrev, is this what we've discussed last time?</div><div><br></div><div>par_scan_state seems like a per-worker-thread data instead of per-generation, but it doesn't seem to affect the performance in this case.</div><div><div><br></div><div>$ hg diff</div><div>diff -r a184ee1d7172 src/share/vm/gc_implementation/parNew/parNewGeneration.cpp</div><div>--- a/src/share/vm/gc_implementation/parNew/parNewGeneration.cpp<span class="" style="white-space:pre"> </span>Thu Jan 08 12:08:22 2015 -0800</div><div>+++ b/src/share/vm/gc_implementation/parNew/parNewGeneration.cpp<span class="" style="white-space:pre"> </span>Wed Jan 14 15:34:57 2015 -0800</div><div>@@ -1194,8 +1194,10 @@</div><div> return real_forwardee(old);</div><div> }</div><div> </div><div>- new_obj = _next_gen->par_promote(par_scan_state->thread_num(),</div><div>- old, m, sz);</div><div>+ if (!par_scan_state->promotion_failed()) {</div><div>+ new_obj = _next_gen->par_promote(par_scan_state->thread_num(),</div><div>+ old, m, sz);</div><div>+ }</div><div> </div><div> if (new_obj == NULL) {</div><div> // promotion failed, forward to self</div></div><div><br></div><div><br></div><div>Thanks,</div><div>Jungwoo</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 26, 2014 at 11:38 PM, Bengt Rutisson <span dir="ltr"><<a href="mailto:bengt.rutisson@oracle.com" target="_blank">bengt.rutisson@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
Hi Kim and Jungwoo,<br>
<br>
Sorry for not replying earlier.<div><div class="h5"><br>
<br>
On 2014-11-24 16:13, Kim Barrett wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Nov 23, 2014, at 10:08 PM, Kim Barrett <<a href="mailto:kim.barrett@oracle.com" target="_blank">kim.barrett@oracle.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Nov 21, 2014, at 3:44 PM, Kim Barrett <<a href="mailto:kim.barrett@oracle.com" target="_blank">kim.barrett@oracle.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Nov 20, 2014, at 9:27 PM, Jungwoo Ha <<a href="mailto:jwha@google.com" target="_blank">jwha@google.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
If we are to put a wrapper around, why not just go with the original change?<br>
I don't see adding a single field on a nearly singleton class is a bad thing.<br>
The changes are well wrapped inside par_promote.<br>
</blockquote>
There are multiple implementations of par_promote, for different<br>
old-space collectors. (par_promote is a virtual function.) At least<br>
some of the others may have similar issues (for example, at a quick<br>
look, ParallelOld appears to have a similar locking structure), and it<br>
seems like all could benefit from this short-circuiting.<br>
<br>
[One of the copy_to_survivor_space_XXX functions (the callers of<br>
par_promote) is used exclusively when CMS is the old-space collector,<br>
the other is used for other old-space collectors.]<br>
</blockquote>
Except I’ve now been reminded that ParNew doesn’t get used with ParallelOld.<br>
In fact, several combinations of old/new collectors were deprecated in jdk8 and<br>
are slated to be removed in jdk9 (some already have), and it looks like ParNew<br>
will only remain in conjunction with CMS, making the split of ParNew’s<br>
copy_to_survivor_space into two variants no longer needed, and one of them<br>
redundant, making this wrapper vs comment duplication vs whatever issue moot.<br>
</blockquote>
To be clear, what I'm suggesting is that, in light of the deprecation<br>
and expected removal of some combinations with ParNew, only<br>
copy_to_survivor_space_<u></u>avoiding_promotion_undo() really needs the<br>
proposed change, and copy_to_survivor_space_with_<u></u>undo() could be left<br>
unmodified.<br>
</blockquote>
<br></div></div>
Yes, this is correct. I sent out the review to remove the support for ParNew+SerialOld yesterday. That removes the copy_to_survivor_space_<u></u>avoiding_promotion_undo() method and folds the copy_to_survivor_space_with_<u></u>undo() method in to copy_to_survivor_space() since that is now the only use case. See the email thread "RFR (L): 8065972: Remove support for ParNew+SerialOld and DefNew+CMS" for more details.<br>
<br>
So, my suggestion would be to hold off with the patch for JDK-8061259 a couple of days until my cleanup has been pushed. That way there will only be one place to add the promotion failure check.<br>
<br>
I agree with Kim that a comment with a short explanation about the intentional (safe) race would be good.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
</blockquote>
<br>
</blockquote></div><br></div>