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

[dean.long at oracle.com]

On 10/23/18 11:04 PM, Mandy Chung wrote:
>
>
> 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.
>

OK, I'll rerun some performance benchmarks and push .5 if the results 
look OK.  David, can you send me your micro-benchmark?
Thanks for the reviews!

dl
> Mandy

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