RFR: 8221394: Clean up ConcurrentGCThread

[Per Liden]

Hi Kim,

On 03/27/2019 07:18 PM, Kim Barrett wrote:
>> 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.

Good, will fix!

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

Ah, I think it happens to work for exactly the reason you mention, but 
we should not rely on that. I'll make it 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 was actually looking into removing it, but backed off when I couldn't 
convince myself that it was safe to do, especially with regards to calls 
to Thread::print_on() on a thread that has terminated. Let's ditch it 
when CMS is gone.

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

Thanks for reviewing, Kim!

/Per