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

[Per Liden]

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.

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

  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   }

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.


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.

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