RFR(S) 8021335: Missing synchronization when reading counters for live threads and peak thread count

[dean.long at oracle.com]

On 10/17/18 2:38 PM, Mandy Chung wrote:
>
>
> On 10/17/18 2:13 PM, dean.long at oracle.com wrote:
>> On 10/17/18 1:41 PM, Mandy Chung wrote:
>>>
>>>
>>> On 10/16/18 7:33 PM, David Holmes wrote:
>>>> Hi Dean,
>>>>
>>>> Thanks for tackling this.
>>>>
>>>> I'm still struggling to fully grasp why we need both the 
>>>> PerfCounters and the regular counters. I get that we have to 
>>>> decrement the live counts before ensure_join() has allowed 
>>>> Thread.join() to return, to ensure that if we then check the number 
>>>> of threads it has dropped by one. But I don't understand why that 
>>>> means we need to manage the thread count in two parts. Particularly 
>>>> as now you don't use the PerfCounter to return the live count, so 
>>>> it makes me wonder what role the PerfCounter is playing as it is 
>>>> temporarily inconsistent with the reported live count? 
>>>
>>> Perf counters were added long time back in JDK 1.4.2 for performance 
>>> measurement before java.lang.management API.  One can use jstat tool 
>>> to monitor VM perf counters of a running VM.   One could look into 
>>> the possibility of deprecating these counters and remove them over time.
>>>
>>>> On 17/10/2018 9:43 AM, dean.long at oracle.com wrote:
>>>> New webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dlong/8021335/webrev.4/
>>>
>>> When the perf counters are updated when a thread is added/removed, 
>>> it's holding Threads_lock.  Are the asserts in 
>>> ThreadService::remove_thread necessary?
>>>
>>
>> Not really.  They were intended to catch the case where the atomic 
>> counters weren't decremented for some reason, not for the perf counters.
>> Should I remove them?
>>
>
> Hmm... when remove_thread is called but decrement_thread_counts has 
> not been called.   It's a bug in thread accounting.  It happens to 
> have the perf counters that can be compared to assert. It seems not 
> obvious.  Setting the perf counters same values as 
> _atomic_threads_count and _atomic_daemon_threads_count makes sense to me.
>
> I would opt for removing the asserts but I can't think of an 
> alternative how to catch the issue you concern about.
>
>>> For clarify, I think we could simply set _live_threads_count to the 
>>> value of _atomic_threads_count and set _daemon_threads_count to the 
>>> value of _atomic_daemon_threads_count.
>>>
>>
>> I think that works, even inside decrement_thread_counts() without 
>> holding the Threads_lock.  If you agree, I'll make that change.
>>
> +1
>

New webrevs, full and incremental:

http://cr.openjdk.java.net/~dlong/8021335/webrev.6/
http://cr.openjdk.java.net/~dlong/8021335/webrev.6.diff/

I like it better without all the asserts too.

dl

> Mandy

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181017/25f53d42/attachment.html>