<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jul 7, 2017, at 4:23 AM, Robbin Ehn <<a href="mailto:robbin.ehn@oracle.com" class="">robbin.ehn@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Hi Roman,</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">On 07/07/2017 12:51 PM, Roman Kennke wrote:</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">Hi Robbin,<br class=""><blockquote type="cite" class=""><br class="">Far down -><br class=""><br class="">On 07/06/2017 08:05 PM, Roman Kennke wrote:<br class=""><blockquote type="cite" class=""><br class=""><blockquote type="cite" class=""><br class="">I'm not happy about this change:<br class=""><br class="">+ ~ParallelSPCleanupThreadClosure() {<br class="">+ // This is here to be consistent with sweeper.cpp<br class="">NMethodSweeper::mark_active_nmethods().<br class="">+ // TODO: Is this really needed?<br class="">+ OrderAccess::storestore();<br class="">+ }<br class=""><br class="">because we're adding an OrderAccess::storestore() to be consistent<br class="">with an OrderAccess::storestore() that's not properly documented<br class="">which is only increasing the technical debt.<br class=""><br class="">So a couple of things above don't make sense to me:<br class=""><br class=""><blockquote type="cite" class="">- sweeper thread runs outside safepoint<br class="">- VMThread (which is doing the nmethod marking in the case that<br class=""> I'm looking at) runs while all other threads (incl. the sweeper)<br class=""> is holding still.<br class=""></blockquote><br class="">and:<br class=""><br class=""><blockquote type="cite" class="">There should be no need for a storestore() (at least in sweeper.cpp...<br class=""></blockquote></blockquote><br class="">Either one or the other are running. Either the VMThread is marking<br class="">nmethods (during safepoint) or the sweeper threads are running (outside<br class="">safepoint). Between the two phases, there is a guaranteed<br class="">OrderAccess::fence() (see safepoint.cpp). Therefore, no storestore()<br class="">should be necessary.<br class=""><br class=""> From Igor's comment I can see how it happened though: Apparently there<br class="">*is* a race in sweeper's own concurrent processing (concurrent with<br class="">compiler threads, as far as I understand). And there's a call to<br class="">nmethod::mark_as_seen_on_stack() after which a storestore() is required<br class="">(as per Igor's explanation). So the logic probably was: we have<br class="">mark_as_seen_on_stack() followed by storestore() here, so let's also put<br class="">a storestore() in the other places that call mark_as_seen_on_stack(),<br class="">one of which happens to be the safepoint cleanup code that we're<br class="">discussing. (why the storestore() hasn't been put right into<br class="">mark_as_seen_on_stack() I don't understand). In short, one storestore()<br class="">really was necessary, the other looks like it has been put there 'for<br class="">consistency' or just conservatively. But it shouldn't be necessary in<br class="">the safepoint cleanup code that we're discussing.<br class=""><br class="">So what should we do? Remove the storestore() for good? Refactor the<br class="">code so that both paths at least call the storestore() in the same<br class="">place? (E.g. make mark_active_nmethods() use the closure and call<br class="">storestore() in the dtor as proposed?)<br class=""></blockquote><br class="">I took a quick look, maybe I'm missing some stuff but:<br class=""><br class="">So there is a slight optimization when not running sweeper to skip<br class="">compiler barrier/fence in stw.<br class=""><br class="">Don't think that matter, so I propose something like:<br class="">- long stack_traversal_mark() { return<br class="">_stack_traversal_mark; }<br class="">- void set_stack_traversal_mark(long l) {<br class="">_stack_traversal_mark = l; }<br class="">+ long stack_traversal_mark() { return<br class="">OrderAccess::load_acquire(&_stack_traversal_mark); }<br class="">+ void set_stack_traversal_mark(long l) {<br class="">OrderAccess::release_store(&_stack_traversal_mark, l); }<br class=""><br class="">Maybe make _stack_traversal_mark volatile also, just as a marking that<br class="">it is concurrent accessed.<br class="">And remove both storestore.<br class=""><br class="">"Also neither of these state variables are volatile in nmethod, so<br class="">even the compiler may reorder the stores"<br class="">Fortunately at least _state is volatile now.<br class=""><br class="">I think _state also should use la/rs semantics instead, but that's<br class="">another story.<br class=""></blockquote>Like this?<br class=""><a href="http://cr.openjdk.java.net/~rkennke/8180932/webrev.12/" class="">http://cr.openjdk.java.net/~rkennke/8180932/webrev.12/</a><br class=""><http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.12/><br class=""></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Yes, exactly, I like this!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Dan? Igor ? Tobias?</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><div><br class=""></div><div>That seems correct.</div><div><br class=""></div><div>igor</div><br class=""><blockquote type="cite" class=""><div class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Thanks Roman!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">BTW I'm going on vacation (5w) in a few hours, but I will follow this thread/changeset to the end!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">/Robbin</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">Roman</blockquote></div></blockquote></div><br class=""></body></html>