<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
tt
{mso-style-priority:99;
font-family:"Courier New";}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;
mso-fareast-language:EN-US;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 2.0cm 70.85pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="DE" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">Hi Hiroshi,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">when looking over the change for the first time, I had missed that the cmpxchg_post_membar is not safe for future enhancements:<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">Please use the condition “else if (order != memory_order_relaxed)” for the sync as in cmpxchg_pre_membar. The code should
still work reliably if somebody adds new enum values.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">I think this is a key property to justify the safety of this change. Adding enum values should not break any platform.
This can be established by using maximum conservative barriers for unknown values.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">If I remember correctly, some reviewers had complained about the acquire barriers. I think it will be better to present
the change without them as this minimizes the impact to Oracle platforms. At least the comment “call acquire for reading fields of new_obj in callers” does not apply to any supported platform (Alpha is not supported) and should be changed.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">I think a precise specification of the required ordering semantics is important. This is the second part which is needed
to justify the safety of this change.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">Best regards,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US">Martin<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span lang="EN-US" style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Hiroshi H Horii [mailto:HORII@jp.ibm.com]
<br>
<b>Sent:</b> Samstag, 29. Oktober 2016 12:37<br>
<b>To:</b> Thomas Schatzl <thomas.schatzl@oracle.com><br>
<b>Cc:</b> David Holmes <david.holmes@oracle.com>; hotspot-compiler-dev <hotspot-compiler-dev@openjdk.java.net>; hotspot-gc-dev@openjdk.java.net; hotspot-runtime-dev@openjdk.java.net; Kim Barrett <kim.barrett@oracle.com>; Doerr, Martin <martin.doerr@sap.com>;
ppc-aix-port-dev@openjdk.java.net<br>
<b>Subject:</b> Re: RFR(M): 8154736: enhancement of cmpxchg and copy_to_survivor for ppc64<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:10.0pt;font-family:"Arial",sans-serif">Hi Thomas,</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> we in the gc team have discussed this change quite a bit internally.</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> Overall, we think this change seems far too risky from both a</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> functional and performance perspective to go into 9 at this time.</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">Thank you for your comments and giving a decision.</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">I completely agree with the decision and would like to keep contributing</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">to this change for future releases.</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> We also think the testing needs to include both functional and</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> performance testing, and the performance testing ought to be using some</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> well-chosen benchmarks. (It was pointed out very early in the</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> discussion of this change that specjbb2013 is deprecated, yet that is</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> the only benchmark that's been reported out.)</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">I see. I will try other workloads and evaluate effects of this change.</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> The most recent change also penalizes current platforms that do not</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> implement the release-CAS with an additional acquire. That might be not</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> an issue for TSO platforms, but others will be affected.</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> </span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> While we think other platforms could quickly adapt to this, this would</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> force that the developer that implements this for other platforms</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> (arm/aarch64) to be stuck with re-analyzing these issues. We</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> do not think this is fair. We think this is a change (or set of</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> changes) that needs to be pushed for all platforms at the same time.</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">Sure. I would like to ask developers for the other platforms to consider</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">this change.</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> There also one (minor) question about the change: why isn't the CAS</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> result value being used for the failing paths of the CAS, rather than</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">> reloaded in copy_to_survivor_space?</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">I believe, the original code also doesn't use the CAS result because</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">the current cas_forward_to doesn't return the CAS result value.</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">bool oopDesc::cas_forward_to(oop p, markOop compare, cmpxchg_memory_order order)</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">I guess, reloading a forwardee is not expensive because CAS fails are rare,</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">then maintenanceability was emphasized.</span><br>
<br>
<span lang="EN-US" style="font-size:10.0pt;font-family:"Arial",sans-serif">"Doerr, Martin" <</span><span style="font-size:10.0pt;font-family:"Arial",sans-serif"><a href="mailto:martin.doerr@sap.com"><span lang="EN-US">martin.doerr@sap.com</span></a></span><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial",sans-serif">>
wrote on 10/21/2016 21:57:42:</span><span lang="EN-US"><br>
</span><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial",sans-serif">> The webrev also contains a logging change in
</span><span lang="EN-US"><br>
</span><span lang="EN-US" style="font-size:10.0pt;font-family:"Arial",sans-serif">> psPromotionManager.inline.hpp which I'm not sure if it's still wanted.</span><span lang="EN-US"><br>
<br>
</span><span style="font-size:10.0pt;font-family:"Arial",sans-serif">For the future discussion, I would like to inform a webrev that doesn't</span><br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">have any changes of log formats.</span><br>
<a href="http://cr.openjdk.java.net/~horii/8154736/webrev.06/"><span style="font-size:10.0pt;font-family:"Arial",sans-serif">http://cr.openjdk.java.net/~horii/8154736/webrev.06/</span></a><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial",sans-serif">Regards,<br>
Hiroshi<br>
-----------------------<br>
Hiroshi Horii, Ph.D.<br>
IBM Research - Tokyo</span><o:p></o:p></p>
</div>
</body>
</html>