RFR: 8221394: Clean up ConcurrentGCThread

[Per Liden]

Thanks Erik!

/Per


On 03/28/2019 04:01 PM, Erik Österlund wrote:
> Hi Per,
> 
> Looks good.
> 
> Thanks,
> /Erik
> 
> On 2019-03-27 14:25, Per Liden 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:
>>>>> 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.
>>
>> 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().
>>
>> Diff: http://cr.openjdk.java.net/~pliden/8221394/webrev.1-diff
>> Full: http://cr.openjdk.java.net/~pliden/8221394/webrev.1
>>
>> /Per
>