RFR (3rd): 8140257: Add support for "gc service threads" to ConcurrentGCThread
Derek White
derek.white at oracle.com
Wed Mar 9 21:22:24 UTC 2016
RFR 3rd version.
Cleans up ConcurrentMarkSweepThread implementation a little more, adds
getters for should_terminate() and has_terminated() to
ConcurrentGCThread (and made fields private).
*Bug*: https://bugs.openjdk.java.net/browse/JDK-8140257
*Webrev*: http://cr.openjdk.java.net/~drwhite/8140257/webrev.04/
*Incremental webrev:*
http://cr.openjdk.java.net/~drwhite/8140257/webrev.v02.v04/webrev/
Thanks!
- Derek
On 3/8/16 10:35 AM, Derek White wrote:
> Hi Per,
>
> On 3/8/16 5:48 AM, Per Liden wrote:
>> Hi Derek,
>>
>> On 2016-03-07 20:36, Derek White wrote:
>>> 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.
>>
>> 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.
>
> 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.
>>> 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?
>>
>> Given my comment above I think you should just leave it as cmst().
>>
>>>> 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.
>>
>> 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.
> OK, I see your point. With the change, the race might still print on a
> terminated cmst, but at least it won't segfault!
>
>>>> 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?
>>
>> Sorry, I cut-n-pasted the wrong version. It doesn't really matter
>> since the new version has the same potential race.
> NP, I just wanted to make sure I wasn't missing something subtle.
>>
>>>> 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");
>>
>> I see, but I would suggest that we instead change the assert to check
>> cmst()->has_terminated() instead. Wouldn't that make more sense?
>
> 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).
>
> I'll get a new webrev out soon.
>
> Thanks again!
>
> - Derek
>>
>> thanks,
>> Per
>>
>>>
>>>> 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/20160309/fdc2f7d4/attachment.htm>
More information about the hotspot-gc-dev
mailing list