RFR (3rd): 8140257: Add support for "gc service threads" to ConcurrentGCThread
Per Liden
per.liden at oracle.com
Fri Mar 11 07:58:59 UTC 2016
Hi Derek,
On 2016-03-10 17:51, Derek White wrote:
> 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/
Looks good, thanks.
Per
>
> - 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