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

[JC Beyler]

Hi Dean,

The new webrev looks much better :) LGTM (not a reviewer though :-)).

Thanks,
Jc
On Wed, Oct 17, 2018 at 3:19 PM <dean.long at oracle.com> wrote:

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

-- 

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