<html><body><p><font size="2" color="#2F2F2F" face="Default San Serif">Hi Martin</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> Kim</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><br><br><font size="2" color="#2F2F2F" face="Default San Serif">Thank you for both of your comments.</font><br><br><font size="2" color="#2F2F2F" face="Default San Serif">I missed the point that oopDesc::forward_to is invoked from several callers. Using OrderAccess:storestore</font><font size="2" color="#2F2F2F" face="Default San Serif">()</font><font size="2" color="#2F2F2F" face="Default San Serif"> before the invocation of forward_to</font><font size="2" color="#2F2F2F" face="Default San Serif">()</font><font size="2" color="#2F2F2F" face="Default San Serif"> would be a great idea</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> thanks.</font><br><br><font size="2" color="#2F2F2F" face="Default San Serif">>I haven't looked carefully at the change</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> though I did find one part
</font><br><font size="2" color="#2F2F2F" face="Default San Serif">>that I don't like. The new test of "order" in forward_to_atomic not
</font><br><font size="2" color="#2F2F2F" face="Default San Serif">>only affects CMS, but also (uselessly) affects G1.</font><br><font size="2" color="#2F2F2F" face="Default San Serif">Please let me confirm your point. You mean I should give memory_order_acq_rel to forward_to_atomic</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> which uses tests as follows to hold the consistent meaning of acquire/release in forward_to_atomic? I agree it is not clear the test with release returns the forwardee with acquire.</font><br><br><font size="2" color="#2F2F2F" face="Default San Serif">oop oopDesc::forward_to_atomic</font><font size="2" color="#2F2F2F" face="Default San Serif">(</font><font size="2" color="#2F2F2F" face="Default San Serif">oop p</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> atomic_memory_order order</font><font size="2" color="#2F2F2F" face="Default San Serif">)</font><font size="2" color="#2F2F2F" face="Default San Serif"> {</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> :</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> while </font><font size="2" color="#2F2F2F" face="Default San Serif">(</font><font size="2" color="#2F2F2F" face="Default San Serif">!oldMark->is_marked</font><font size="2" color="#2F2F2F" face="Default San Serif">())</font><font size="2" color="#2F2F2F" face="Default San Serif"> {</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> if </font><font size="2" color="#2F2F2F" face="Default San Serif">(</font><font size="2" color="#2F2F2F" face="Default San Serif">order == memory_order_acq_rel</font><font size="2" color="#2F2F2F" face="Default San Serif">)</font><font size="2" color="#2F2F2F" face="Default San Serif"> {</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> curMark = cas_set_mark_raw</font><font size="2" color="#2F2F2F" face="Default San Serif">(</font><font size="2" color="#2F2F2F" face="Default San Serif">forwardPtrMark</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> oldMark</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> memory_order_release</font><font size="2" color="#2F2F2F" face="Default San Serif">);</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> } else {</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> curMark = cas_set_mark_raw</font><font size="2" color="#2F2F2F" face="Default San Serif">(</font><font size="2" color="#2F2F2F" face="Default San Serif">forwardPtrMark</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> oldMark</font><font size="2" color="#2F2F2F" face="Default San Serif">,</font><font size="2" color="#2F2F2F" face="Default San Serif"> order</font><font size="2" color="#2F2F2F" face="Default San Serif">);</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> }</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> }</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> :</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> }</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> if </font><font size="2" color="#2F2F2F" face="Default San Serif">(</font><font size="2" color="#2F2F2F" face="Default San Serif">order == memory_order_acq_rel</font><font size="2" color="#2F2F2F" face="Default San Serif">)</font><font size="2" color="#2F2F2F" face="Default San Serif"> {</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> return forwardee_acquire</font><font size="2" color="#2F2F2F" face="Default San Serif">();</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> }</font><br><font size="2" color="#2F2F2F" face="Default San Serif"> return forwardee</font><font size="2" color="#2F2F2F" face="Default San Serif">();</font><br><font size="2" color="#2F2F2F" face="Default San Serif">}</font><br><br><br><font size="2" face="Default San Serif">Best regards,</font><br><font size="2" face="Default San Serif">--</font><br><font size="2" face="Default San Serif">Michihiro,</font><br><font size="2" face="Default San Serif">IBM Research - Tokyo</font><br><br><img width="16" height="16" src="cid:1__=8FBB0853DFB8136F8f9e8a93df938690918c8FB@" border="0" alt="Inactive hide details for Kim Barrett ---2018/07/04 05:41:02---> On Jul 3, 2018, at 4:25 AM, Michihiro Horie <HORIE@jp.ibm.com>"><font size="2" color="#424282">Kim Barrett ---2018/07/04 05:41:02---> On Jul 3, 2018, at 4:25 AM, Michihiro Horie <HORIE@jp.ibm.com> wrote: ></font><br><br><font size="2" color="#5F5F5F">From: </font><font size="2">Kim Barrett <kim.barrett@oracle.com></font><br><font size="2" color="#5F5F5F">To: </font><font size="2">Michihiro Horie <HORIE@jp.ibm.com></font><br><font size="2" color="#5F5F5F">Cc: </font><font size="2">"Doerr, Martin" <martin.doerr@sap.com>, "hotspot-gc-dev@openjdk.java.net" <hotspot-gc-dev@openjdk.java.net>, Gustavo Romero <gromero@linux.vnet.ibm.com></font><br><font size="2" color="#5F5F5F">Date: </font><font size="2">2018/07/04 05:41</font><br><font size="2" color="#5F5F5F">Subject: </font><font size="2">Re: 8205908: Unnecessarily strong memory barriers in ParNewGeneration::copy_to_survivor_space</font><br><hr width="100%" size="2" align="left" noshade style="color:#8091A5; "><br><br><br><tt><font size="2">> On Jul 3, 2018, at 4:25 AM, Michihiro Horie <HORIE@jp.ibm.com> wrote:<br>> <br>> Hi Martin,<br>> <br>> Thanks a lot for your review. Sure, we need an OK from a CMS expert. Following is the new webrev:<br>> </font></tt><tt><font size="2"><a href="http://cr.openjdk.java.net/~mhorie/8205908/webrev.01/">http://cr.openjdk.java.net/~mhorie/8205908/webrev.01/</a></font></tt><tt><font size="2"><br>> <br>> >Seems like a user of the forwardee needs to rely on memory_order_consume in the current implementation. I guess it will be appreciated that you’re fixing this.<br>> Thank you for pointing out this issue in the original implementation. I newly inserted a release at "2.4. Set new_obj as forwardee [L1142]".<br>> <br>> Improvement of critical-jOPS in SPECjbb2015 was 10%, which is still a big number.<br>> <br>> <br>> Best regards,<br>> --<br>> Michihiro,<br>> IBM Research - Tokyo<br><br>CMS was deprecated in JDK 9, and has been on maintenance life-support<br>for some time. This complex-to-review performance enhancement was<br>proposed less than 48 hours before JDK 11 FC, and didn't receive any<br>reviews until after FC. Because of these factors, I don't think it<br>should be included in JDK 11. And if CMS gets removed in JDK 12 (I<br>don't know if that will happen), then this change would be rendered<br>entirely moot.<br><br>I haven't looked carefully at the change, though I did find one part<br>that I don't like. The new test of "order" in forward_to_atomic not<br>only affects CMS, but also (uselessly) affects G1.<br><br>I'm not going to be able to look at this carefully soon, as JDK 11 bug<br>fixing has a higher priority for me. Since I think CMS might soon not<br>be an issue, I'd really rather not look at it at all. I think this<br>change needs not just a CMS-expert reviewer, but someone who is<br>willing to maintain CMS (including any potential bug tail from this<br>change).<br><br><br><br></font></tt><br><br><BR>
</body></html>