RFR: 8140257: Add support for "gc service threads" to ConcurrentGCThread
Per Liden
per.liden at oracle.com
Tue Mar 8 10:48:06 UTC 2016
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.
>
> 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.
>> 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.
>> 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?
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