<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 14 (filtered medium)">
<style><!--
/* Font Definitions */
@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:11.0pt;
font-family:"Calibri","sans-serif";
mso-fareast-language:EN-US;}
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;}
span.EmailStyle17
{mso-style-type:personal-compose;
font-family:"Calibri","sans-serif";
color:windowtext;}
.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"><o:p> </o:p></p>
<p class="MsoNormal"><span lang="EN-US">Hi Volker,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">thanks for your comment.Indeed, this was my first idea for a fix. However, there are more than 50 callers of load_heap_oop, plus some generated by preprocessor defines like this one:<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">#define DO_OOP_WORK_IMPL(closureName) \<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> template <class T> inline void closureName##Closure::do_oop_work(T* p) { \<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> T heap_oop = oopDesc::load_heap_oop(p); \<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> if (!oopDesc::is_null(heap_oop)) { \<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> oop obj = oopDesc::decode_heap_oop_not_null(heap_oop); \<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> do_oop(obj); \<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> } \<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"> }<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">We would have to decide for each of the callers, if there might be a concurrency problem or not.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">Do you think, it is worth the effort and the risk of missing one?<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">Regards,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">Axel<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">On 22.06.2015 at 18:17 Volker Simonis wrote:<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> Hi Axel,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> the change looks good, but shouldn't we better provide two versions of<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> load_heap_oop()<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> inline oop oopDesc::load_heap_oop(oop* p)<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> inline oop oopDesc::load_heap_oop(volatile oop* p)<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> such that the callers can decide on their own which versions they require?<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> Or do the callers of load_heap_oop() not always know if there exist<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> concurrent mutator threads?<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> Regards,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> Volker<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> On Mon, Jun 22, 2015 at 5:33 PM, Siebenborn, Axel<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">> <axel.siebenborn@sap.com> wrote:<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> Hi,<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> we have seen crashes with G1 during concurrent root region scan.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> It turned out, that the reason for the crashes was reloading an oop after a null check.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> The compiler may legally do this, as the pointer is not declared as volatile.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> The code runs concurrently to mutator threads.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> template <class T><o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> inline void G1RootRegionScanClosure::do_oop_nv(T* p) {<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> T heap_oop = oopDesc::load_heap_oop(p); // 1. Load oop<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> if (!oopDesc::is_null(heap_oop)) {<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> oop obj = oopDesc::decode_heap_oop_not_null(heap_oop); // 2. Compiler decides to re-load the oop<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> HeapRegion* hr = _g1h->heap_region_containing((HeapWord*) obj);<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> _cm->grayRoot(obj, obj->size(), _worker_id, hr);<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> }<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> }<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> We have seen the problem on AIX with the xlC compiler. However, we have seen similar optimizations on other platforms and other platforms.<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> As this code pattern is used quite often, I would suggest a global fix in oopDesc::load_heap_oop(p).<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> Bug: https://bugs.openjdk.java.net/browse/JDK-8129440<o:p></o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>> Webrev: http://cr.openjdk.java.net/~asiebenborn/8129440/webrev.00/<o:p></o:p></span></p>
<p class="MsoNormal">>><o:p> </o:p></p>
<p class="MsoNormal">>> Thanks,<o:p></o:p></p>
<p class="MsoNormal">>> Axel<o:p></o:p></p>
<p class="MsoNormal">>><o:p> </o:p></p>
<p class="MsoNormal">>><o:p> </o:p></p>
<p class="MsoNormal">>><o:p> </o:p></p>
</div>
</body>
</html>