<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<br>
<br>
Hi Jungwoo,<br>
<br>
<div class="moz-cite-prefix">On 2014-10-29 23:51, Jungwoo Ha wrote:<br>
</div>
<blockquote
cite="mid:CA+n_jhit9GZY_skbBHmWmX6ZqXm=6H9yes-yA2P_R+N5wxOSSQ@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div>PTAL </div>
<div><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Erasbold/8061259/webrev.01/"
target="_blank" style="line-height:19.7999992370605px">http://cr.openjdk.java.net/~rasbold/8061259/webrev.01/</a><br>
</div>
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">I've looked a bit at the
webrev. A couple of comments:<br>
</div>
<div bgcolor="#FFFFFF" text="#000000"> <br>
Why do you use OrderAccess methods for writing and reading
the _has_promo_failed flag in has_promo_failed() and
set_promot_failed() ?<br>
</div>
</blockquote>
<div><br>
</div>
</div>
<div class="gmail_quote">
<div>I think that has no effect on x86, but I assumed that
processors with weak memory model may want ordering of
set/reset/has call.</div>
</div>
</blockquote>
<br>
You don't need the OrderAccess methods for the weak memory models
here either. You just race on reading the variable and if you see
the "wrong" value you eventually take a lock (which will order all
memory accesses) to read the variable properly.<br>
<br>
By removing the use of OrderAccess you can make
ConcurrentMarkSweepGeneration::_has_promotion_failed a bool instead
of a juint which simplifies the code a bit.<br>
<br>
<blockquote
cite="mid:CA+n_jhit9GZY_skbBHmWmX6ZqXm=6H9yes-yA2P_R+N5wxOSSQ@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">Can we write out the
full word "promotion" instead of just "promo" in the
variables and methods?<br>
</div>
</blockquote>
<div><br>
</div>
</div>
<div class="gmail_quote">
<div>Done.</div>
</div>
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> Can we change the name
of the flag from UseCMSFastPromotionFailure to
CMSFastPromotionFailure? Most CMS flags start with CMS and I
don't think we need the "Use" prefix.<br>
</div>
</blockquote>
<div><br>
</div>
</div>
<div class="gmail_quote">
<div>Done.</div>
</div>
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">What do you think about
making the flag true by default? At least for JDK 9. If we
decide to backport to JDK 8 or 7 it might be a good idea to
keep the default value as false.<br>
</div>
</blockquote>
<div><br>
</div>
</div>
<div class="gmail_quote">
<div>Done.</div>
<div>Let me know if there is anything for me to do to backport
to JDK8 and 7.</div>
</div>
</blockquote>
<br>
I think this fix would be worth backporting to JDK 8. I don't think
there is much action required on your side. I created a backport bug
for JDK 8 just to make sure that we don't forget it. It will be a
little while before the 8 update repos are in a state to accept
enhancements again. So, it is nice to have the backport bug to keep
track of this.<br>
<br>
Backporting to JDK 7 requires some more work. Unless you have good
arguments for why it is important to backport this to JDK 7 I don't
think it is worth doing.<br>
<br>
<blockquote
cite="mid:CA+n_jhit9GZY_skbBHmWmX6ZqXm=6H9yes-yA2P_R+N5wxOSSQ@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">Did you find the
information provided by _fast_promo_failure_hitcount useful
for debugging? If it not too useful I would consider
removing it since it is cluttering up the code a bit.</div>
</blockquote>
<div><br>
</div>
</div>
<div class="gmail_quote">
<div>I removed it.</div>
<div>It was useful to for development, but I think it is no
longer needed.</div>
</div>
</blockquote>
<br>
Great. Thanks.<br>
<br>
One more comment. This code comment appears in two places just after
we have taken the lock.<br>
<br>
3365 if (CMSFastPromotionFailure &&
has_promotion_failed()) {<br>
3366 // Caller must have checked already without
synchronization.<br>
3367 // Check again here while holding the lock.<br>
3368 return NULL;<br>
3369 }<br>
<br>
There is actually really no requirement that the caller must have
checked has_promotion_failed() before calling the method. That's
just an optimization. I think the first comment can be skipped and
we just leave the second comment "// Check again here while holding
the lock.". I would also suggest moving that comment up to the line
just before the if statement.<br>
<br>
Thanks,<br>
Bengt<br>
<br>
<br>
<blockquote
cite="mid:CA+n_jhit9GZY_skbBHmWmX6ZqXm=6H9yes-yA2P_R+N5wxOSSQ@mail.gmail.com"
type="cite">
<div class="gmail_quote">
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
</div>
</blockquote>
<br>
</body>
</html>