RFR(XS): 8202564: java/lang/management/ThreadMXBean/ThreadCounts.java fails

[Robbin Ehn]

Hi, sorry, I saw this now, just FYI:

There is still an issue regarding this:
   static jlong get_daemon_thread_count()      { return 
_daemon_threads_count->get_value() - _exiting_daemon_threads_count; }

Many threads can have exited between the loads. And thus this method can return 
a negative number if I'm not mistaken.

https://bugs.openjdk.java.net/browse/JDK-8202839

/Robbin

On 2018-05-09 09:45, Doerr, Martin wrote:
> Hi Vladimir and Daniel,
> 
> thank you for reviewing and pushing.
> 
> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Daniel D. Daugherty [mailto:daniel.daugherty at oracle.com]
> Sent: Dienstag, 8. Mai 2018 18:24
> To: Vladimir Kozlov <vladimir.kozlov at oracle.com>; Doerr, Martin <martin.doerr at sap.com>; hotspot-dev Source Developers <hotspot-dev at openjdk.java.net>
> Subject: Re: RFR(XS): 8202564: java/lang/management/ThreadMXBean/ThreadCounts.java fails
> 
>   > http://cr.openjdk.java.net/~mdoerr/8202564_thread_cnt/webrev.00/
> 
> src/hotspot/share/services/threadService.cpp
>       You have moved the decrement of _exiting_daemon_threads_count
> 
>       L125:   if (daemon) {
>       L126:     Atomic::dec(&_exiting_daemon_threads_count);
>       L127:   }
> 
>       above this check:
> 
>       L129:   if (thread->is_hidden_from_external_view() ||
>       L130:       thread->is_jvmti_agent_thread()) {
> 
>       and away from this other decrement:
> 
>       L136:
> _daemon_threads_count->set_value(_daemon_threads_count->get_value() - 1);
> 
>       That makes sense because the original increment of
> _exiting_daemon_threads_count
>       happens here:
> 
>       L140 void ThreadService::current_thread_exiting(JavaThread* jt) {
>       L141   assert(jt == JavaThread::current(), "Called by current thread");
>       L142   Atomic::inc(&_exiting_threads_count);
>       L143
>       L144   oop threadObj = jt->threadObj();
>       L145   if (threadObj != NULL &&
> java_lang_Thread::is_daemon(threadObj)) {
>       L146     Atomic::inc(&_exiting_daemon_threads_count);
>       L147   }
>       L148 }
> 
>       and is not guarded by the hidden thread or jvmti check. For other
>       folks that are reading closely, these two checks are equivalent:
> 
>         if (daemon) {
> 
>       and:
> 
>         if (threadObj != NULL && java_lang_Thread::is_daemon(threadObj)) {
> 
>       Very nice catch!
> 
> 
> I'm just guessing here, but I think the recent CompilerThread changes
> now allow CompilerThreads to go thru the ThreadService::remove_thread()
> code path which exposed this bug. So if there was a late query about
> the number of daemon threads and a CompilerThread had exited, then the
> count would be off. However, if the CompilerThread exited a little bit
> later, then the test would pass...
> 
> Thumbs up!
> 
> Dan
> 
> 
> 
> On 5/8/18 12:03 PM, Vladimir Kozlov wrote:
>> Hi Martin,
>>
>> Thank you for finding the cause and fixing it. Looks good.
>>
>> Changing to hotspot-dev alias since it is not compiler code. Let other
>> people look on it.
>>
>> Thanks,
>> Vladimir
>>
>> On 5/8/18 6:50 AM, Doerr, Martin wrote:
>>> Hi Vladimir,
>>>
>>> I've fixed the thread count issue.
>>>
>>> "_exiting_daemon_threads_count" just needs to get decremented at the
>>> right place (like "_exiting_threads_count").
>>>
>>> Please review:
>>>
>>> http://cr.openjdk.java.net/~mdoerr/8202564_thread_cnt/webrev.00/
>>>
>>> Best regards,
>>>
>>> Martin
>>>
>