RFR: 8221394: Clean up ConcurrentGCThread
per.liden at oracle.com
Wed Mar 27 13:25:52 UTC 2019
On 3/27/19 8:35 AM, Per Liden wrote:
> 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
>> 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.
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.
To move this review forward, I reverted back to using Terminator_lock,
and changed the notify() in WatcherThread to be a notify_all().
More information about the hotspot-gc-dev