RFR: 8221394: Clean up ConcurrentGCThread

[Per Liden]

Hi Kim,

On 3/27/19 5:50 AM, Kim Barrett wrote:
>> On Mar 25, 2019, at 4:54 AM, Per Liden <per.liden at oracle.com> wrote:
>>
>> ConcurrentGCThread currently uses Terminator_lock to coordinate shutdown. This is dubious, as this lock is shared by many threads, some of which calls notify() rather than notify_all() to signal that something happened. It's also potentially inefficient as we might be waking up more threads than needed. Each ConcurrentGCThread should have it's own monitor for this.
>>
>> This patch introduces a separate monitor for each instance of ConcurrentGCThread and does some general code cleanups in that class.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221394
>> Webrev: http://cr.openjdk.java.net/~pliden/8221394/webrev.0
>>
>> /Per
> 
> The Terminator_lock is only waited upon by the thread(s) that are
> stopping the ConcurrentGCThreads and the WatcherThread.  That's
> normally only one thread, during the shutdown process in before_exit().
> And before_exit() has special code to ensure no more than one thread
> gets that far.  So there's no efficiency issue with waking up more
> threads than needed.
> 
> I don't have any objection to changing the notify() calls to
> notify_all() (including the one by the WatcherThread).  The cases
> where notify() is correct and better than notify_all() are, in my
> experience, few and far between, and generally needs some commentary
> to explain why.
> 
> But I don't think all these new monitors are needed.  The caller of
> before_exit() is effectively going to go through a sequence of
> threads, calling stop() on each, and waiting for each to signal that
> it has stopped (possibly waiting on the Terminator_lock).

I think you're right. When I first went through the uses of 
Terminator_lock I got the sense that several threads could be waiting on 
it, but I think I was wrong there.

The other reason why I'd like having a monitor per ConcurrentGCThread is 
encapsulation. Having ConcurrentGCThread be self contained and not 
depend on an shared lock (that might be used/abused by some other 
thread) seems like a good thing. One could argue that the current use of 
Terminator_lock is an implementation details that has leaked out for no 
good reason, and the overhead of a monitor per thread is negligible.

> 
> A possibly useful change would be to split the current stop() into a
> request and a wait, so that, for example, the implementations of
> CollectedHeap::stop() could request all and then wait for each, rather
> than looping on request then wait for each, which might give some
> improvement to shutdown time.  If we were doing that there would be
> some benefit to separate monitors, though a shared counter of pending
> requests would work as well.

I like that idea. I'll give that a shot and come back.

Thanks for looking at the patch!

/Per