<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">Hi Per,<br>
<br>
Thanks for the comments. More below...<br>
<br>
On 3/7/16 5:25 AM, Per Liden wrote:<br>
</div>
<blockquote cite="mid:56DD5704.5030301@oracle.com" 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 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 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 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/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
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 _cmst == NULL indicated that termination had completed seems
to be gone now from ConcurrentMarkSweepThread::stop(), and I don't
see any other uses of this.
<br>
</blockquote>
The "_cmst != NULL" on line 89 isn't to catch termination, but to
protect the dereference "_cmst->_has_terminated". _cmst could be
NULL if not initialized.<br>
<br>
What I was trying to do here is preserve the existing behavior for
readers of the _cmst flag. Originally "_cmst != NULL" meant that the
cmst thread had started and had not terminated. In the new code,
_cmst never reverts to NULL at termination, so I changed the cmst()
function to catch both conditions.<br>
<br>
Maybe "cmst()" is a poor name - the usual convention is that a
getter is a trivial wrapper around a private/protected field. Maybe
"active_cmst()" or "running_cmst()" would be better?<br>
<blockquote cite="mid:56DD5704.5030301@oracle.com" type="cite">
Also, the above code looks potentially racy, if the thread
terminates at the same time (not sure about all contexts where
this could be called).
<br>
</blockquote>
I don't follow this. Can you give some more detail? I don't see a <i>new</i>
race here - _has_terminated is set in the same place as as _cmst
used to be cleared.<br>
<br>
I can't promise there weren't any old races. For example cmst()
could go NULL between lines 166 and 167 below in the new code, or
_cmst could go NULL between lines 191 and 192 in the old code below.<br>
<blockquote cite="mid:56DD5704.5030301@oracle.com" 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
code at 165-169. So I'm not sure if you were just showing the new
code or comparing old vs. new code here?<br>
<blockquote cite="mid:56DD5704.5030301@oracle.com" 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 missing here.
<br>
</blockquote>
<br>
The other callers of cmst() are in assertions. Arguably code like
the following is really trying to ensure that the cmst thread has
started and has not terminated.<br>
<tt> assert(ConcurrentMarkSweepThread::cmst() != NULL,</tt><tt><br>
</tt><tt> "CMS thread must be running");</tt><br>
<br>
<blockquote cite="mid:56DD5704.5030301@oracle.com" 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
_should_terminate. _has_terminated shouldn't need a getter after
the changes related to my other comment.
<br>
</blockquote>
<br>
OK, that sounds good. I think a "has_terminated()" getter would be
useful though.<br>
<br>
Thanks for the review!<br>
<br>
- Derek<br>
<blockquote cite="mid:56DD5704.5030301@oracle.com" type="cite">
<br>
cheers.
<br>
/Per
<br>
<br>
<blockquote type="cite">*Incremental 1 vs 2*:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/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 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 class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~drwhite/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 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>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</body>
</html>