Nice catch!<br>PTAL<div><a href="http://cr.openjdk.java.net/~rasbold/8061259/webrev.03/" target="_blank" style="line-height:19.7999992370605px">http://cr.openjdk.java.net/~rasbold/8061259/webrev.03/</a></div><br><div class="gmail_quote">On Thu Oct 30 2014 at 1:33:04 PM Bengt Rutisson <<a href="mailto:bengt.rutisson@oracle.com">bengt.rutisson@oracle.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div><br>
Hi Jungwoo,</div></div><div bgcolor="#FFFFFF" text="#000000"><div><br>
<br>
On 10/30/14 6:24 PM, Jungwoo Ha wrote:<br>
</div></div><div bgcolor="#FFFFFF" text="#000000"><div></div>
<blockquote type="cite">PTAL<br>
<a href="http://cr.openjdk.java.net/%7Erasbold/8061259/webrev.02/" style="font-family:arial,sans-serif;line-height:19.5px" target="_blank">http://cr.openjdk.java.net/~rasbold/8061259/webrev.02/</a><br>
</blockquote>
<br>
Thanks! Looks good except for one detail.<br>
<br>
1125 bool set_promotion_failed() { _has_promotion_failed = 1; }<br>
1126 bool reset_promotion_failed() { _has_promotion_failed = 0; }<br>
<br>
Since _has_promotion_failed is now a bool I don't think we should be
assigning 1 and 0 to it. We should be using true and false.<br>
<br>
Other than that it looks good to me.<br>
<br>
Thanks!</div><div bgcolor="#FFFFFF" text="#000000"><br>
Bengt</div><div bgcolor="#FFFFFF" text="#000000"><br>
<br>
<blockquote type="cite">
<div><br>
</div>
<br>
<div class="gmail_quote">On Thu Oct 30 2014 at 12:28:19 AM Bengt
Rutisson <<a href="mailto:bengt.rutisson@oracle.com" target="_blank">bengt.rutisson@oracle.com</a>>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> <br>
Hi again,<br>
<br>
One more minor thing.<br>
<br>
The methods has_promotion_failed(), set_promotion_failed()
and reset_promotion_failed() are protected but they could be
made private instead.<br>
<br>
Thanks,<br>
Bengt</div>
<div bgcolor="#FFFFFF" text="#000000"><br>
<br>
<div>On 2014-10-30 08:09, Bengt Rutisson wrote:<br>
</div>
<blockquote type="cite"> <br>
<br>
Hi Jungwoo,<br>
<br>
<div>On 2014-10-29 23:51, Jungwoo Ha wrote:<br>
</div>
<blockquote type="cite">
<div class="gmail_quote">
<div>PTAL </div>
<div><a href="http://cr.openjdk.java.net/%7Erasbold/8061259/webrev.01/" style="line-height:19.7999992370605px" target="_blank">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 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 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 type="cite">
<div class="gmail_quote">
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</div></blockquote></div>