<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></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 6, 2017, at 3:14 AM, Tobias Hartmann <<a href="mailto:tobias.hartmann@oracle.com" class="">tobias.hartmann@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Hi,<br class=""><br class="">On 05.07.2017 20:30, Daniel D. Daugherty wrote:<br class=""><blockquote type="cite" class="">JDK-8132849 is assigned to Tobias; it would be good to get Tobias'<br class="">review of this fix also.<br class=""></blockquote><br class="">Thanks for the notification. The sweeper/safepoint changes look good to me!<br class=""><br class=""><blockquote type="cite" class="">src/share/vm/runtime/sweeper.cpp<br class=""> L205: // TODO: Is this really needed?<br class=""> L206: OrderAccess::storestore();<br class=""> That's a good question. Looks like that storestore() was<br class=""> added by this changeset:<br class=""><br class=""> $ hg log -r 5357 src/share/vm/runtime/sweeper.cpp<br class=""> changeset: 5357:510fbd28919c<br class=""> user: anoll<br class=""> date: Fri Sep 27 10:50:55 2013 +0200<br class=""> summary: 8020151: PSR:PERF Large performance regressions when code cache is filled<br class=""><br class=""> The changeset is not small and it looks like two<br class=""> OrderAccess::storestore() calls were added (and one<br class=""> load_ptr_acquire() was deleted):<br class=""><br class=""> $ hg diff -r 5356 -r 5357 | grep OrderAccess<br class=""> + OrderAccess::storestore();<br class=""> - nmethod *code = (nmethod *)OrderAccess::load_ptr_acquire(&_code);<br class=""> + OrderAccess::storestore();<br class=""><br class=""> It could be that the storestore() is matching an existing<br class=""> OrderAccess operation or it could have been added in an<br class=""> abundance of caution. We definitely need a Compiler team<br class=""> person to take a look here.<br class=""></blockquote><br class="">Unfortunately, I'm also not sure if that barrier is required. Looking at the old RFR thread:<br class=""><a href="http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2013-September/011588.html" class="">http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2013-September/011588.html</a><br class=""><br class="">It seems that Igor V. suggested this:<br class="">"You definitely need a store-store barrier for non-TSO architectures after the mark_as_seen_on_stack() call on line 1360. Otherwise it still can be reordered by the CPU with respect to the following state assignment. Also neither of these state variables are volatile in nmethod, so even the compiler may reorder the stores."<br class="">http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2013-September/011729.html<br class=""><br class="">The requested OrderAccess::storestore() was added to nmethod::make_not_entrant_or_zombie() but seems like Albert also added one to NMethodSweeper::mark_active_nmethods().<br class=""><br class="">I'll ping Igor, maybe he knows more.<br class=""></div></div></blockquote><div><br class=""></div><div><br class=""></div><div>I think the reason is explained in the comment:</div><div><br class=""></div><div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// Must happen before state change. Otherwise we have a race condition in</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// <span style="text-decoration: underline" class="">nmethod</span>::can_not_entrant_be_converted(). I.e., a method can immediately</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// transition its state from 'not_entrant' to '<span style="text-decoration: underline" class="">zombie</span>' without having to wait</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// for stack scanning.</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> <span style="color: #931a68" class="">if</span> (state == <span style="color: #0326cc" class="">not_entrant</span>) {</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> mark_as_seen_on_stack();</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> <span style="color: #006141" class="">OrderAccess</span>::storestore();</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> }</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; min-height: 15px;" class=""><br class=""></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// Change state</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> <span style="color: #0326cc" class="">_state</span> = state;</div><div class=""><br class=""></div><div class="">Although can_not_entrant_be_converted() is now called can_convert_to_zombie(). The scenario can so like this:</div><div class="">1. We’re setting the state to not_entrant. But the _state assignment happens before setting the traversal count in mark_as_seen_on_stack().</div><div class="">2. While we’re doing this, the sweeper scans nmethods and is in process_compiled_method():</div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""><br class=""></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> } <span style="color: #931a68" class="">else</span> <span style="color: #931a68" class="">if</span> (cm-><span style="text-decoration: underline" class="">is_not_entrant</span>()) {</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// If there are no current activations of this method on the</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// stack we can safely convert it to a <span style="text-decoration: underline" class="">zombie</span> method</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> <span style="color: #931a68" class="">if</span> (cm->can_convert_to_zombie()) {</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// Clear ICStubs to prevent back patching stubs of <span style="text-decoration: underline" class="">zombie</span> or flushed</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// <span style="text-decoration: underline" class="">nmethods</span> during the next <span style="text-decoration: underline" class="">safepoint</span> (see ICStub::finalize).</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> {</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> <span style="color: #006141" class="">MutexLocker</span> cl(CompiledIC_lock);</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> cm->clear_ic_stubs();</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> }</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco; color: rgb(78, 144, 114);" class=""><span style="color: #000000" class=""> </span>// Code cache state change is tracked in make_zombie()</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Monaco;" class=""> cm->make_zombie();</div></div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">So if state change happens before setting the traversal mark, the sweeper can go ahead and make it a zombie.</div><div class=""><br class=""></div><div class=""><br class=""></div><div class="">Makes sense? Or am I missing something?</div><div class=""><br class=""></div><div class="">igor</div></div><div><br class=""></div><br class=""><blockquote type="cite" class=""><div class=""><div class=""><br class="">Thanks,<br class="">Tobias<br class=""></div></div></blockquote></div><br class=""></body></html>