<font size=2 face="sans-serif">Dear Kim, David, and all,</font><br><br><font size=2 face="sans-serif">Thank you for your comments. </font><br><br><font size=2 face="sans-serif">I created a new webrev. I added memory_order_release
as a new enum of cmpxchg_memory_order (atomic.hpp) and use it to update
forwardees.  </font><br><br><a href=http://cr.openjdk.java.net/~horii/8154736/webrev.04/><font size=2 color=blue face="sans-serif">http://cr.openjdk.java.net/~horii/8154736/webrev.04/</font></a><br><br><font size=2 face="sans-serif">Originally, two sync were called before
and after cmpxchg in ppc. With this change, one of them is reduced. Though
one sync still remains, performance will be improved.</font><br><br><font size=2 face="sans-serif">Could you give your comments on this
new webrev?</font><br><br><font size=2 face="sans-serif">Regards,<br>Hiroshi<br>-----------------------<br>Hiroshi Horii, Ph.D.<br>IBM Research - Tokyo<br></font><br><br><tt><font size=2>Kim Barrett <kim.barrett@oracle.com> wrote on
10/07/2016 07:16:28:<br><br>> From: Kim Barrett <kim.barrett@oracle.com></font></tt><br><tt><font size=2>> To: David Holmes <david.holmes@oracle.com></font></tt><br><tt><font size=2>> Cc: Hiroshi H Horii/Japan/IBM@IBMJP, hotspot-compiler-dev
<hotspot-<br>> compiler-dev@openjdk.java.net>, Tim Ellison <br>> <Tim_Ellison@uk.ibm.com>, "ppc-aix-port-dev@openjdk.java.net"
<ppc-<br>> aix-port-dev@openjdk.java.net>, Michihiro Horie/Japan/IBM@IBMJP,
<br>> "hotspot-gc-dev@openjdk.java.net" <hotspot-gc-dev@openjdk.java.net>,<br>> "hotspot-runtime-dev@openjdk.java.net" <hotspot-runtime-dev@openjdk.java.net></font></tt><br><tt><font size=2>> Date: 10/07/2016 07:17</font></tt><br><tt><font size=2>> Subject: Re: RFR(M): 8154736: enhancement of
cmpxchg and <br>> copy_to_survivor for ppc64</font></tt><br><tt><font size=2>> <br>> > On Oct 5, 2016, at 9:36 PM, David Holmes <david.holmes@oracle.com>
wrote:<br>> > <br>> > On 5/10/2016 10:36 AM, Hiroshi H Horii wrote:<br>> >> Dear David,<br>> >> <br>> >> Thank you for your comments.<br>> >> <br>> >> I just used to think that it may be better that copy_to_survivor_space<br>> >> doesn't return forwardee if CAS was failed in order to prevent
from<br>> >> reading fields in forwardee. But as you pointed, this extends
fix for<br>> >> this topic.<br>> >> <br>> >> I removed two NULL assignments from the previous wevrev.<br>> >> </font></tt><a href=http://cr.openjdk.java.net/~horii/8154736/webrev.03/><tt><font size=2>http://cr.openjdk.java.net/~horii/8154736/webrev.03/</font></tt></a><tt><font size=2><br>> > <br>> > Which simply takes us back to where we were. It may not be safe
<br>> for the caller of those methods to access the fields of the returned<br>> "forwardee".<br>> > <br>> > Sorry but I'm not seeing anything here that justifies removing
the<br>> barriers from the cas in this code. GC lurkers feel free to jump in
<br>> here - this is your code afterall! ;-)<br>> > <br>> > David<br>> > -----<br>> <br>> Using a CAS with memory_order_relaxed in copy_to_survivor_space seems<br>> to me to be extremely fragile and hard to reason about.  The
places<br>> where that copied object might escape to and be examined seem to be<br>> myriad.  And not only do we need to worry about them today, but
also<br>> for future maintenance.  Even if it can modified and shown to
be<br>> correct today, it would be very easy to intoduce a bug later, as<br>> should be obvious from the various issues pointed out so far during<br>> this review.<br>> <br>> The key issue here is that we copy obj into new_obj, and then make<br>> new_obj accessible to other threads via the CAS.  Those other
threads<br>> might attempt to access data in new_obj.  This suggests the CAS
ought<br>> to have at least a release fence to ensure the copy is complete before<br>> the CAS is performed.  No amount of fencing on the read side
(such as<br>> in the work stealing) can remove that need.<br>> <br>> And that might be all that is needed.  On the post-CAS side,
we load<br>> the forwardee and then load values from it.  I thik we can use<br>> implicit consume with dependent loads (except on Alpha) plus the<br>> suggested release fence to get the desired effect.  (If not,
use an<br>> acquire form of forwardee()?)<br>> <br>> I'm not certain that just a release fence is sufficient (I'm less<br>> familiar with ParallelGC than I'd like for looking at something like<br>> this), but I'm pretty sure I wouldn't want to go any weaker than that.<br>> <br>> <br></font></tt><BR>