RFR: 8221394: Clean up ConcurrentGCThread

[Kim Barrett]

> On Mar 27, 2019, at 9:25 AM, Per Liden <per.liden at oracle.com> wrote:
> 
> Hi again,
> 
> On 3/27/19 8:35 AM, Per Liden wrote:
>> Hi Kim,
>> On 3/27/19 5:50 AM, Kim Barrett wrote:
>>> A possibly useful change would be to split the current stop() into a
>>> request and a wait, […]
>> I like that idea. I'll give that a shot and come back.
> 
> I started going down this path, but fairly quickly ran out of steam as it becomes a much larger thing. Let's save the re-factoring for another day.

No problem, it was just an idea. I'm not even certain it's a good idea.

> To move this review forward, I reverted back to using Terminator_lock, and changed the notify() in WatcherThread to be a notify_all().
> 
> Diff: http://cr.openjdk.java.net/~pliden/8221394/webrev.1-diff
> Full: http://cr.openjdk.java.net/~pliden/8221394/webrev.1
> 
> /Per

------------------------------------------------------------------------------ 
src/hotspot/share/gc/shared/concurrentGCThread.hpp
  49   bool should_terminate();
  50   bool has_terminated();

[pre-existing] These should be const.

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/concurrentGCThread.cpp
  63   OrderAccess::release_store(&_should_terminate, true);

I think removal of the surrounding lock and just using a release_store
here makes the update of _should_terminate no longer inter-thread
happens before the call to stop_service.  That doesn't seem right,
though it may not currently matter in practice; I think all the
implementations of stop_service start by locking some mutex.  Maybe
this should be a release_store_fence?

------------------------------------------------------------------------------
src/hotspot/share/gc/shared/concurrentGCThread.cpp
  78 bool ConcurrentGCThread::has_terminated() {
  79   return OrderAccess::load_acquire(&_has_terminated);
  80 }

Just an aside: if it weren't for some CMS oddities I think we wouldn't
even need this function and the associated special handling of the
variable. CMS is the only caller, and I'm not sure it's a "good" caller.

------------------------------------------------------------------------------

I don't need a new webrev for any of the suggested changes.