<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
Hi Jungwoo,<br>
<br>
Thanks for bringing this thread to life again.<br>
<br>
<div class="moz-cite-prefix">On 2015-01-15 00:39, Jungwoo Ha wrote:<br>
</div>
<blockquote
cite="mid:CA+n_jhhpeWiFamYDNtO+aogr9T=t3mWzPGOFikomWr0iWtUQgw@mail.gmail.com"
type="cite">
<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>
</blockquote>
<br>
Do you prefer the per thread check instead of the "global" state?
Not sure which one is best. You say there is no performance
difference between the two versions. Is that based on your test
program that measures how long a GC that gets promotion failure
takes? What about overall performance for normal GCs? The
par_scan_state->promotion_failed() looks more expensive than just
reading _promotion_failed.<br>
<br>
Also, in your original patch you were guarding this check with a
flag. Do you still want to do that?<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<blockquote
cite="mid:CA+n_jhhpeWiFamYDNtO+aogr9T=t3mWzPGOFikomWr0iWtUQgw@mail.gmail.com"
type="cite">
<div dir="ltr">
<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 moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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
moz-do-not-send="true"
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_avoiding_promotion_undo()
really needs the<br>
proposed change, and copy_to_survivor_space_with_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_avoiding_promotion_undo() method and
folds the copy_to_survivor_space_with_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>
</blockquote>
<br>
</body>
</html>