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

Per Liden per.liden at oracle.com
Thu Mar 10 08:51:51 UTC 2016


Hi Derek,

On 2016-03-09 22:48, Derek White wrote:
> 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 for fixing. Looks good, just noticed one minor thing:

genCollectedHeap.cpp
--------------------
1284     ConcurrentMarkSweepThread::stop_all();

could be just:

   ConcurrentMarkSweepThread::cmst()->stop();

to align with the approach you used for the strdedup thread, and 
stop_all() could then be removed. I leave it up to you to decide if you 
want to keep it as is or remove stop_all(). I don't need to see a new 
webrev.

thanks,
Per


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


More information about the hotspot-gc-dev mailing list