RFR: 8140257: Add support for "gc service threads" to ConcurrentGCThread

Derek White derek.white at oracle.com
Tue Mar 8 15:35:00 UTC 2016


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 
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>



More information about the hotspot-gc-dev mailing list