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

[Mandy Chung]

On 10/23/18 10:34 PM, David Holmes wrote:
> On 24/10/2018 8:30 AM, dean.long at oracle.com wrote:
>> On 10/23/18 2:51 PM, David Holmes wrote:
>>> Hi Dean,
>>>
>>> On 24/10/2018 4:05 AM, dean.long at oracle.com wrote:
>>>> On 10/23/18 9:46 AM, dean.long at oracle.com wrote:
>>>>> On 10/22/18 3:31 PM, David Holmes wrote:
>>>>>> Sorry Dean I'm concerned about a thread termination bottleneck 
>>>>>> with this. A simple microbenchmark that creates 500,000 threads 
>>>>>> that have to run and terminate, shows a 15+% slowdown on my Linux 
>>>>>> box. I tried to find some kind of real benchmarks that covers 
>>>>>> thread termination but couldn't see anything specific.
>>>>>>
>>>>>> Can you at least run this through our performance system to see 
>>>>>> if any of the regular benchmarks are affected.
>>>>>>
>>>>>
>>>>> OK, but even if the regular benchmarks don't show a difference, 
>>>>> I'd feel better if microbenchmarks were not affected.  What if I 
>>>>> go back to the original approach and add locking:
>>>>>
>>>>>    static jlong get_live_thread_count()        { MutexLocker 
>>>>> mu(Threads_lock); return _live_threads_count->get_value() - 
>>>>> _exiting_threads_count; }
>>>>>    static jlong get_daemon_thread_count()      { MutexLocker 
>>>>> mu(Threads_lock); return _daemon_threads_count->get_value() - 
>>>>> _exiting_daemon_threads_count; }
>>>>>
>>>>> along with the other cleanups around is_daemon and is_hidden_thread?
>>>>>
>>>>
>>>> Some micro-benchmarks like SecureRandomBench show a regression with 
>>>> webrev.7.  I could go back to webrev.5 and then we shouldn't need 
>>>> any locking in the get_*() functions.
>>>
>>> I don't see version 5 discussed but I took a look and it seems okay. 
>>
>> Mandy had questions about the asserts in .5, and it seemed like we 
>> could just set the perf counters to the same value as the atomic 
>> counters, which resulted in .6.  I think the only problem with .6 is 
>> that I set the perf counters in decrement_thread_counts without the 
>> lock.  If I "sync" the perf counters to the atomic counters only in 
>> add_thread and remove_thread, with the lock, then it's about the same 
>> as .5, but without the asserts and parallel inc/dec.  If anyone likes 
>> the sound of that, I can send out a new webrev.  Or we can go with 
>> webrev.5.
>
> I'm not sure what the concern was with the asserts - if they mis-fire 
> we'll know soon enough. So I'm okay with .5
>
>>> My only query with that version is that it appears the actual 
>>> perfCounters no longer get read by anything - is that the case?
>>>
>>
>> There does seem to be code that references them, through their name 
>> string.  That's a difference interface that I'm not familiar with, so 
>> I didn't want to break it.
>
> Right - they can be accessed directly through other means. I was 
> concerned that the perfCounter was out of sync with 
> get_live_thread-count() but that's already the case so not an issue.
>
> If all tests and benchmarks are happy I say go with version .5
>

I have no objection to version .5 if most people prefer that.  My 
comment was that I don't think the asserts are necessary.

Mandy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20181023/b3ff1899/attachment-0001.html>