<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 7/6/17 10:53 AM, Roman Kennke wrote:<br>
<blockquote type="cite"
cite="mid:58f2278e-b95c-4ec2-4f7d-9fefa3a281e4@redhat.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<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"
moz-do-not-send="true">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>
</blockquote>
<tt>Â <br>
If we are going to have the OrderAccess::storestore() calls, then<br>
we have to have a proper comment explaining why they are needed.<br>
<br>
Unfortunately, the OrderAccess::storestore() call that was added<br>
by anoll to src/share/vm/runtime/sweeper.cpp back in 2013 was not<br>
properly documented and we're bumping into that with this review.<br>
<br>
I'm not happy about this change:<br>
<br>
+Â ~ParallelSPCleanupThreadClosure() {<br>
+Â Â Â // This is here to be consistent with sweeper.cpp<br>
NMethodSweeper::mark_active_nmethods().<br>
+Â Â Â // TODO: Is this really needed?<br>
+Â Â Â OrderAccess::storestore();<br>
+Â }<br>
<br>
because we're adding an OrderAccess::storestore() to be consistent<br>
with an OrderAccess::storestore() that's not properly documented<br>
which is only increasing the technical debt.<br>
<br>
So a couple of things above don't make sense to me:<br>
<br>
> - sweeper thread runs outside safepoint<br>
> - VMThread (which is doing the nmethod marking in the case
that<br>
>Â Â I'm looking at) runs while all other threads (incl. the
sweeper)<br>
>Â Â is holding still.<br>
<br>
and:<br>
<br>
> There should be no need for a storestore() (at least in
sweeper.cpp...<br>
<br>
If the sweeper thread is running "outside safepoint", then how is<br>
the sweeper thread "holding still" while the VMThread is doing the<br>
nmethod marking? Those two points are contradictory.<br>
<br>
If the sweeper thread is indeed executing outside a safepoint,
then<br>
a storestore() is needed for its memory changes to be seen by the<br>
VMThread which is doing things in parallel. That means that the<br>
comment that sweeper.cpp doesn't need the storestore() is also<br>
contradictory.<br>
<br>
So what do you mean by this comment:<br>
<br>
> - sweeper thread runs outside safepoint<br>
<br>
and once we know that we can figure out the rest...<br>
<br>
Dan<br>
<br>
<br>
</tt>
<blockquote type="cite"
cite="mid:58f2278e-b95c-4ec2-4f7d-9fefa3a281e4@redhat.com"> <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>
</blockquote>
<br>
</body>
</html>