<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Ok, so I guess I need a sponsor for
this now:<br>
<br>
<a moz-do-not-send="true"
href="http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.12/">http://cr.openjdk.java.net/~rkennke/8180932/webrev.12/</a><br>
<br>
Roman<br>
<br>
Am 07.07.2017 um 20:09 schrieb Igor Veresov:<br>
</div>
<blockquote type="cite"
cite="mid:F54B29FF-C4A3-48DA-BB4E-2F6DEED753A3@oracle.com">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<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=""
moz-do-not-send="true">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/%7Erkennke/8180932/webrev.12/"
class="" moz-do-not-send="true">http://cr.openjdk.java.net/~rkennke/8180932/webrev.12/</a><br
class="">
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.12/"><http://cr.openjdk.java.net/%7Erkennke/8180932/webrev.12/></a><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="">
</blockquote>
<p><br>
</p>
</body>
</html>