RFR (3rd): 8140257: Add support for "gc service threads" to ConcurrentGCThread

Derek White derek.white at oracle.com
Wed Mar 9 21:48:29 UTC 2016


Forgot to add testing:

Ran jprt and rbt with  -a -XX:+UseConcMarkSweepGC --job hs-nightly-runtime.

On 3/9/16 4:22 PM, Derek White wrote:
> 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: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20160309/4d1753a5/attachment-0001.html>


More information about the hotspot-gc-dev mailing list