<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Forgot to add testing:<br>
<br>
Ran jprt and rbt with -a -XX:+UseConcMarkSweepGC --job
hs-nightly-runtime.<br>
<br>
On 3/9/16 4:22 PM, Derek White wrote:<br>
</div>
<blockquote cite="mid:56E09410.4090506@oracle.com" type="cite">
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">RFR 3rd version.<br>
<br>
Cleans up ConcurrentMarkSweepThread implementation a little
more, adds getters for should_terminate() and has_terminated()
to ConcurrentGCThread (and made fields private).<br>
<br>
<b>Bug</b>: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8140257">https://bugs.openjdk.java.net/browse/JDK-8140257</a><br>
<b>Webrev</b>: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.04/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.04/</a>
<br>
<b>Incremental webrev:</b> <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.v02.v04/webrev/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.v02.v04/webrev/</a><br>
<br>
Thanks!<br>
<br>
- Derek<br>
<br>
On 3/8/16 10:35 AM, Derek White wrote:<br>
</div>
<blockquote cite="mid:56DEF124.9030004@oracle.com" type="cite">Hi
Per, <br>
<br>
On 3/8/16 5:48 AM, Per Liden wrote: <br>
<blockquote type="cite">Hi Derek, <br>
<br>
On 2016-03-07 20:36, Derek White wrote: <br>
<blockquote type="cite">Hi Per, <br>
<br>
Thanks for the comments. More below... <br>
<br>
On 3/7/16 5:25 AM, Per Liden wrote: <br>
<blockquote type="cite">Hi Derek, <br>
<br>
On 2016-03-03 18:14, Derek White wrote: <br>
<blockquote type="cite">RFR 2nd version. <br>
<br>
New version is focused on making
ConcurrentMarkSweepThread a proper <br>
subclass of ConcurrentGCThread, especially related to
sharing the same <br>
initialization and termination protocols. See
incremental webrev <br>
<a moz-do-not-send="true" class="moz-txt-link-rfc2396E"
href="http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.01.v.02/"><http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.01.v.02/></a>
for <br>
details. <br>
<br>
* Move CMS-specific code to
run_service()/stop_service(), inherit <br>
run()/stop() methods. <br>
* Removed
ConcurrentMarkSweepThread::_should_terminate, use
inherited <br>
_should_terminate instead. <br>
* Use the inherited _has_terminated flag instead of
_cmst to denote <br>
termination. Users call cmst() instead of reading
_cmst. <br>
* Change ConcurrentMarkSweepThread::start() and stop()
to match G1's <br>
handling - assume stop() only called after start(),
so <br>
ConcurrentGCThread objects have been created. <br>
o CMS and G1 start() methods called in same place:
<br>
Universe::Initialize_heap(), and the stop()
methods are called <br>
in same place: before_exit(), so they have the
same lifetimes <br>
for their ConcurrentGCThreads. <br>
<br>
<br>
*Bug*: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8140257">https://bugs.openjdk.java.net/browse/JDK-8140257</a>
<br>
*Webrev*: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.02/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.02/</a>
<br>
</blockquote>
<br>
Thanks for doing this cleanup. Looks good overall, just
two minor <br>
comments: <br>
<br>
concurrentMarkSweepThread.hpp <br>
----------------------------- <br>
<br>
88 inline static ConcurrentMarkSweepThread* cmst() { <br>
89 if (_cmst != NULL &&
!_cmst->_has_terminated) { <br>
90 return _cmst; <br>
91 } <br>
92 return NULL; <br>
93 } <br>
<br>
Checking _has_terminated here seems a bit strange. The use
case where <br>
_cmst == NULL indicated that termination had completed
seems to be <br>
gone now from ConcurrentMarkSweepThread::stop(), and I
don't see any <br>
other uses of this. <br>
</blockquote>
The "_cmst != NULL" on line 89 isn't to catch termination,
but to <br>
protect the dereference "_cmst->_has_terminated". _cmst
could be NULL if <br>
not initialized. <br>
<br>
What I was trying to do here is preserve the existing
behavior for <br>
readers of the _cmst flag. Originally "_cmst != NULL" meant
that the <br>
cmst thread had started and had not terminated. In the new
code, _cmst <br>
never reverts to NULL at termination, so I changed the
cmst() function <br>
to catch both conditions. <br>
</blockquote>
<br>
I'm questioning if adding logic to cmst() to cater for the
termination condition is a good idea. I think callers which
want to check is the thread has terminated should be doing
cmst()->has_terminated() instead. <br>
</blockquote>
<br>
OK, I'll make cmst() a normal trivial getter. I think most of
the callers will end up doing "if (cmst() != NULL &&
!cmst()->has_terminated())", but that's OK. <br>
<blockquote type="cite">
<blockquote type="cite">Maybe "cmst()" is a poor name - the
usual convention is that a getter is <br>
a trivial wrapper around a private/protected field. Maybe <br>
"active_cmst()" or "running_cmst()" would be better? <br>
</blockquote>
<br>
Given my comment above I think you should just leave it as
cmst(). <br>
<br>
<blockquote type="cite">
<blockquote type="cite">Also, the above code looks
potentially racy, if the thread terminates <br>
at the same time (not sure about all contexts where this
could be <br>
called). <br>
</blockquote>
I don't follow this. Can you give some more detail? I don't
see a /new/ <br>
race here - _has_terminated is set in the same place as as
_cmst used to <br>
be cleared. <br>
<br>
I can't promise there weren't any old races. For example
cmst() could go <br>
NULL between lines 166 and 167 below in the new code, or
_cmst could go <br>
NULL between lines 191 and 192 in the old code below. <br>
</blockquote>
<br>
Right, this is the race I'm referring to. Your change didn't
introduce it, but we have the opportunity to remove it if
cmst() just return _cmst as I suggest. <br>
</blockquote>
OK, I see your point. With the change, the race might still
print on a terminated cmst, but at least it won't segfault! <br>
<br>
<blockquote type="cite">
<blockquote type="cite">
<blockquote type="cite"> 165 void
ConcurrentMarkSweepThread::print_all_on(outputStream* st)
{ <br>
166 if (cmst() != NULL) { <br>
167 cmst()->print_on(st); <br>
168 st->cr(); <br>
169 } <br>
<br>
189 void
ConcurrentMarkSweepThread::threads_do(ThreadClosure* tc) {
<br>
190 assert(tc != NULL, "Null ThreadClosure"); <br>
191 if (_cmst != NULL) { <br>
192 tc->do_thread(_cmst); <br>
193 } <br>
</blockquote>
<br>
BTW, the lines 189-193 are old code, but not old versions of
the new <br>
code at 165-169. So I'm not sure if you were just showing
the new code <br>
or comparing old vs. new code here? <br>
</blockquote>
<br>
Sorry, I cut-n-pasted the wrong version. It doesn't really
matter since the new version has the same potential race. <br>
</blockquote>
NP, I just wanted to make sure I wasn't missing something
subtle. <br>
<blockquote type="cite"> <br>
<blockquote type="cite">
<blockquote type="cite">I'd suggest we keep: <br>
<br>
89 static ConcurrentMarkSweepThread* cmst() {
return _cmst; } <br>
<br>
and never set _cmst to NULL, unless there's some very good
reason I'm <br>
missing here. <br>
</blockquote>
<br>
The other callers of cmst() are in assertions. Arguably code
like the <br>
following is really trying to ensure that the cmst thread
has started <br>
and has not terminated. <br>
assert(ConcurrentMarkSweepThread::cmst() != NULL, <br>
"CMS thread must be running"); <br>
</blockquote>
<br>
I see, but I would suggest that we instead change the assert
to check cmst()->has_terminated() instead. Wouldn't that
make more sense? <br>
</blockquote>
<br>
OK, as long as we check both cmst() != NULL and
cmst()->has_terminated() (unless I can ensure that the caller
runs after initialization is complete). <br>
<br>
I'll get a new webrev out soon. <br>
<br>
Thanks again! <br>
<br>
- Derek <br>
<blockquote type="cite"> <br>
thanks, <br>
Per <br>
<br>
<blockquote type="cite"> <br>
<blockquote type="cite">concurrentGCThread.hpp <br>
---------------------- <br>
<br>
34 protected: <br>
35 bool volatile _should_terminate; <br>
36 bool _has_terminated; <br>
<br>
Please make these private and add a protected getter for <br>
_should_terminate. _has_terminated shouldn't need a getter
after the <br>
changes related to my other comment. <br>
</blockquote>
<br>
OK, that sounds good. I think a "has_terminated()" getter
would be <br>
useful though. <br>
<br>
Thanks for the review! <br>
<br>
- Derek <br>
<blockquote type="cite"> <br>
cheers. <br>
/Per <br>
<br>
<blockquote type="cite">*Incremental 1 vs 2*: <br>
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.01.v.02/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.01.v.02/</a>
<br>
<br>
*Tests*: <br>
- jprt <br>
- Aurora Perf (including Startup3-Server-CMS,
Footprint3-Server-CMS) <br>
- Aurora Test "hs-nightly-gc-cms". <br>
<br>
Thanks for looking! <br>
- Derek <br>
<br>
On 2/26/16 11:51 PM, Derek White wrote: <br>
<blockquote type="cite">I'm working on a new webrev, so
please hold off on reviewing. <br>
<br>
Some offline comments from Kim suggest trying another
approach. Any <br>
other approach :-) He rightly pointed out the poor
match between the <br>
new code and ConcurrentMarkSweepThread is pretty ugly.
<br>
<br>
Two other options I'm looking at are either having <br>
ConcurrentMarkSweepThread not subclass from
ConcurrentGCThread, or <br>
(more likely) refactor ConcurrentMarkSweepThread to
use the common <br>
termination protocol instead of doing it's own thing.
The approach of <br>
adding an intermediate class that handles the common
code being <br>
factored out was rejected in review comments for
"8138920". <br>
<br>
- Derek <br>
<br>
On 2/26/16 11:56 AM, Derek White wrote: <br>
<blockquote type="cite">*Background*: <br>
ConcurrentGCThread provides incomplete support for
an initialization <br>
and termination protocol for GC threads, so missing
parts are <br>
duplicated in almost all subclasses. <br>
<br>
*Fix*: <br>
Move duplicated run(), and stop() methods up from
subclasses <br>
ConcurrentG1RefineThread, ConcurrentMarkThread,
G1StringDedupThread, <br>
and G1YoungRemSetSamplingThread to
ConcurrentGCThread, as well as <br>
declare virtual methods run_service() and
stop_service. <br>
<br>
Note that ConcurrentMarkSweepThread is the odd-ball
subclass. It <br>
implements it's own termination protocol, it
provides it's own run() <br>
and stop() methods, and does not use run_service()
and stop_service(). <br>
<br>
*Bug*: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8140257">https://bugs.openjdk.java.net/browse/JDK-8140257</a>
<br>
*Webrev*: <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.01/">http://cr.openjdk.java.net/~drwhite/8140257/webrev.01/</a>
<br>
<br>
*Tests*: jprt, Aurora "nightly" run (I think this is
OK) <br>
<a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://aurora.ru.oracle.com/faces/Batch.xhtml?batchName=1325690.VMSQE.adhoc.JPRT">http://aurora.ru.oracle.com/faces/Batch.xhtml?batchName=1325690.VMSQE.adhoc.JPRT</a>
<br>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>