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

Derek White derek.white at oracle.com
Thu Mar 10 16:51:38 UTC 2016


Thanks Per,

I made the change and spun a new webrev for the curious, and ran through 
jprt.

Any comments Kim?

*Bug*: https://bugs.openjdk.java.net/browse/JDK-8140257
*Webrev*: http://cr.openjdk.java.net/~drwhite/8140257/webrev.05/
*Incremental webrev:*
http://cr.openjdk.java.net/~drwhite/8140257/webrev.v04.v05/

  - Derek


On 3/10/16 3:51 AM, Per Liden wrote:
> 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