RFR: 8221394: Clean up ConcurrentGCThread

[Kim Barrett]

> 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

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.

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

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.