RFR: 8140257: Add support for "gc service threads" to ConcurrentGCThread
Derek White
derek.white at oracle.com
Mon Mar 7 19:36:40 UTC 2016
Hi Per,
Thanks for the comments. More below...
On 3/7/16 5:25 AM, Per Liden wrote:
> Hi Derek,
>
> On 2016-03-03 18:14, Derek White wrote:
>> RFR 2nd version.
>>
>> New version is focused on making ConcurrentMarkSweepThread a proper
>> subclass of ConcurrentGCThread, especially related to sharing the same
>> initialization and termination protocols. See incremental webrev
>> <http://cr.openjdk.java.net/%7Edrwhite/8140257/webrev.01.v.02/> for
>> details.
>>
>> * Move CMS-specific code to run_service()/stop_service(), inherit
>> run()/stop() methods.
>> * Removed ConcurrentMarkSweepThread::_should_terminate, use inherited
>> _should_terminate instead.
>> * Use the inherited _has_terminated flag instead of _cmst to denote
>> termination. Users call cmst() instead of reading _cmst.
>> * Change ConcurrentMarkSweepThread::start() and stop() to match G1's
>> handling - assume stop() only called after start(), so
>> ConcurrentGCThread objects have been created.
>> o CMS and G1 start() methods called in same place:
>> Universe::Initialize_heap(), and the stop() methods are called
>> in same place: before_exit(), so they have the same lifetimes
>> for their ConcurrentGCThreads.
>>
>>
>> *Bug*: https://bugs.openjdk.java.net/browse/JDK-8140257
>> *Webrev*: http://cr.openjdk.java.net/~drwhite/8140257/webrev.02/
>
> Thanks for doing this cleanup. Looks good overall, just two minor
> comments:
>
> concurrentMarkSweepThread.hpp
> -----------------------------
>
> 88 inline static ConcurrentMarkSweepThread* cmst() {
> 89 if (_cmst != NULL && !_cmst->_has_terminated) {
> 90 return _cmst;
> 91 }
> 92 return NULL;
> 93 }
>
> 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.
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.
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.
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?
> 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).
I don't follow this. Can you give some more detail? I don't see a /new/
race here - _has_terminated is set in the same place as as _cmst used to
be cleared.
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.
> 165 void ConcurrentMarkSweepThread::print_all_on(outputStream* st) {
> 166 if (cmst() != NULL) {
> 167 cmst()->print_on(st);
> 168 st->cr();
> 169 }
>
> 189 void ConcurrentMarkSweepThread::threads_do(ThreadClosure* tc) {
> 190 assert(tc != NULL, "Null ThreadClosure");
> 191 if (_cmst != NULL) {
> 192 tc->do_thread(_cmst);
> 193 }
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?
> I'd suggest we keep:
>
> 89 static ConcurrentMarkSweepThread* cmst() { return _cmst; }
>
> and never set _cmst to NULL, unless there's some very good reason I'm
> missing here.
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.
assert(ConcurrentMarkSweepThread::cmst() != NULL,
"CMS thread must be running");
> concurrentGCThread.hpp
> ----------------------
>
> 34 protected:
> 35 bool volatile _should_terminate;
> 36 bool _has_terminated;
>
> 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.
OK, that sounds good. I think a "has_terminated()" getter would be
useful though.
Thanks for the review!
- Derek
>
> cheers.
> /Per
>
>> *Incremental 1 vs 2*:
>> http://cr.openjdk.java.net/~drwhite/8140257/webrev.01.v.02/
>>
>> *Tests*:
>> - jprt
>> - Aurora Perf (including Startup3-Server-CMS, Footprint3-Server-CMS)
>> - Aurora Test "hs-nightly-gc-cms".
>>
>> Thanks for looking!
>> - Derek
>>
>> On 2/26/16 11:51 PM, Derek White wrote:
>>> I'm working on a new webrev, so please hold off on reviewing.
>>>
>>> Some offline comments from Kim suggest trying another approach. Any
>>> other approach :-) He rightly pointed out the poor match between the
>>> new code and ConcurrentMarkSweepThread is pretty ugly.
>>>
>>> Two other options I'm looking at are either having
>>> ConcurrentMarkSweepThread not subclass from ConcurrentGCThread, or
>>> (more likely) refactor ConcurrentMarkSweepThread to use the common
>>> termination protocol instead of doing it's own thing. The approach of
>>> adding an intermediate class that handles the common code being
>>> factored out was rejected in review comments for "8138920".
>>>
>>> - Derek
>>>
>>> On 2/26/16 11:56 AM, Derek White wrote:
>>>> *Background*:
>>>> ConcurrentGCThread provides incomplete support for an initialization
>>>> and termination protocol for GC threads, so missing parts are
>>>> duplicated in almost all subclasses.
>>>>
>>>> *Fix*:
>>>> Move duplicated run(), and stop() methods up from subclasses
>>>> ConcurrentG1RefineThread, ConcurrentMarkThread, G1StringDedupThread,
>>>> and G1YoungRemSetSamplingThread to ConcurrentGCThread, as well as
>>>> declare virtual methods run_service() and stop_service.
>>>>
>>>> Note that ConcurrentMarkSweepThread is the odd-ball subclass. It
>>>> implements it's own termination protocol, it provides it's own run()
>>>> and stop() methods, and does not use run_service() and stop_service().
>>>>
>>>> *Bug*: https://bugs.openjdk.java.net/browse/JDK-8140257
>>>> *Webrev*: http://cr.openjdk.java.net/~drwhite/8140257/webrev.01/
>>>>
>>>> *Tests*: jprt, Aurora "nightly" run (I think this is OK)
>>>> http://aurora.ru.oracle.com/faces/Batch.xhtml?batchName=1325690.VMSQE.adhoc.JPRT
>>>>
>>>
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20160307/6dc4b884/attachment.htm>
More information about the hotspot-gc-dev
mailing list