<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Am 06.07.2017 um 18:47 schrieb Igor
Veresov:<br>
</div>
<blockquote type="cite"
cite="mid:20E06CEC-38CA-41AE-99DB-17EF22A3C5CC@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<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=""
moz-do-not-send="true">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="" moz-do-not-send="true">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="">
<a class="moz-txt-link-freetext" href="http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2013-September/011729.html">http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2013-September/011729.html</a><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>
</div>
</blockquote>
<br>
I have probably not fully digged the code. As far as I can see:<br>
- sweeper thread runs outside safepoint<br>
- VMThread (which is doing the nmethod marking in the case that I'm
looking at) runs while all other threads (incl. the sweeper) is
holding still.<br>
<br>
In between we have a guaranteed fence().<br>
<br>
There should be no need for a storestore() (at least in
sweeper.cpp... in nmethod.cpp it seems to actually make sense as you
pointed out above). *However* it doesn't really hurt to
OrderAccess::storestore() there... so play it conservative and leave
it in, as RFR'd in my last patch?<br>
<br>
Roman<br>
<br>
<blockquote type="cite"
cite="mid:20E06CEC-38CA-41AE-99DB-17EF22A3C5CC@oracle.com">
<div>
<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="">
</blockquote>
<p><br>
</p>
</body>
</html>