<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
Changes look good.<br>
<br>
Reviewed.<br>
<br>
Thanks for the fix.<br>
<br>
Jon<br>
<br>
<div class="moz-cite-prefix">On 11/4/2014 8:16 AM, Jungwoo Ha wrote:<br>
</div>
<blockquote
cite="mid:CA+n_jhggT6S59oQbqWE=AptufHJ4Sni_KUWpwyHq4HNH=6Oyrw@mail.gmail.com"
type="cite"><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Erasbold/8061259/webrev.04/"
target="_blank" style="line-height:19.7999992370605px">http://cr.openjdk.java.net/~rasbold/8061259/webrev.04/</a><br>
Thanks for the review.<br>
<div class="gmail_quote">On Tue Nov 04 2014 at 1:27:44 AM Bengt
Rutisson <<a moz-do-not-send="true"
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"> <br>
Hi Jungwoo,<br>
<br>
One final nit.<br>
<br>
The two setters don't need a return value. They could be
void.<br>
<br>
1125 bool set_promotion_failed() { _has_promotion_failed
= true; }<br>
1126 bool reset_promotion_failed() { _has_promotion_failed
= false; }</div>
<div bgcolor="#FFFFFF" text="#000000"><br>
<br>
Bengt</div>
<div bgcolor="#FFFFFF" text="#000000"><br>
<br>
<div>On 2014-11-03 18:16, Jungwoo Ha wrote:<br>
</div>
<blockquote type="cite">Nice catch!<br>
PTAL
<div><a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Erasbold/8061259/webrev.03/"
style="line-height:19.7999992370605px" target="_blank">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 moz-do-not-send="true"
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">
<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">
<blockquote type="cite">PTAL<br>
<a moz-do-not-send="true"
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
moz-do-not-send="true"
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 moz-do-not-send="true"
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>
</blockquote>
<br>
</div>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>