RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)

[Daniel D. Daugherty]

On 6/29/19 12:06 PM, Daniil Titov wrote:
> Hi Serguei and David,
>
> Serguei is right, ThreadTable::find_thread(java_tid) cannot  return a JavaThread with an unmatched java_tid.
>
> Please find a new version of the fix that includes the changes Serguei suggested.
>
> Regarding the concern about the maintaining the thread table when it may never even be queried, one of
> the options could be to add ThreadTable ::isEnabled flag, set it to "false" by default, and wrap the calls to the thread table
> in ThreadsSMRSupport add_thread() and remove_thread() methods to check this flag.
>
> When ThreadsList::find_JavaThread_from_java_tid() is called for the first time it could check if ThreadTable ::isEnabled
> Is on and if not then set it on and populate the thread table with all existing threads from the thread list.

I have the same concerns as David H. about this new ThreadTable.
ThreadsList::find_JavaThread_from_java_tid() is only called from code
in src/hotspot/share/services/management.cpp so I think that table
needs to enabled and populated only if it is going to be used.

I've taken a look at the webrev below and I see that David has
followed up with additional comments. Before I do a crawl through
code review for this, I would like to see the ThreadTable stuff
made optional and David's other comments addressed.

Another possible optimization is for callers of
find_JavaThread_from_java_tid() to save the calling thread's
tid value before they loop and if the current tid == saved_tid
then use the current JavaThread* instead of calling
find_JavaThread_from_java_tid() to get the JavaThread*.

Dan

>
> Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
>
> Thanks!
> --Daniil
>
> From: <serguei.spitsyn at oracle.com>
> Organization: Oracle Corporation
> Date: Friday, June 28, 2019 at 7:56 PM
> To: Daniil Titov <daniil.x.titov at oracle.com>, OpenJDK Serviceability <serviceability-dev at openjdk.java.net>, "hotspot-runtime-dev at openjdk.java.net" <hotspot-runtime-dev at openjdk.java.net>, "jmx-dev at openjdk.java.net" <jmx-dev at openjdk.java.net>
> Subject: Re: RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
>
> Hi Daniil,
>
> I have several quick comments.
>
> The indent in the hotspot c/c++ files has to be 2, not 4.
>
> https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/src/hotspot/share/runtime/threadSMR.cpp.frames.html
> 614 JavaThread* ThreadsList::find_JavaThread_from_java_tid(jlong java_tid) const {
>   615     JavaThread* java_thread = ThreadTable::find_thread(java_tid);
>   616     if (java_thread == NULL && java_tid == PMIMORDIAL_JAVA_TID) {
>   617         // ThreadsSMRSupport::add_thread() is not called for the primordial
>   618         // thread. Thus, we find this thread with a linear search and add it
>   619         // to the thread table.
>   620         for (uint i = 0; i < length(); i++) {
>   621             JavaThread* thread = thread_at(i);
>   622             if (is_valid_java_thread(java_tid,thread)) {
>   623                 ThreadTable::add_thread(java_tid, thread);
>   624                 return thread;
>   625             }
>   626         }
>   627     } else if (java_thread != NULL && is_valid_java_thread(java_tid, java_thread)) {
>   628         return java_thread;
>   629     }
>   630     return NULL;
>   631 }
>   632 bool ThreadsList::is_valid_java_thread(jlong java_tid, JavaThread* java_thread) {
>   633     oop tobj = java_thread->threadObj();
>   634     // Ignore the thread if it hasn't run yet, has exited
>   635     // or is starting to exit.
>   636     return (tobj != NULL && !java_thread->is_exiting() &&
>   637             java_tid == java_lang_Thread::thread_id(tobj));
>   638 }
>
>   615     JavaThread* java_thread = ThreadTable::find_thread(java_tid);
>
>    I'd suggest to rename find_thread() to find_thread_by_tid().
>
> A space is missed after the comma:
>    622 if (is_valid_java_thread(java_tid,thread)) {
>
> An empty line is needed before L632.
>
> The name 'is_valid_java_thread' looks wrong (or confusing) to me.
> Something like 'is_alive_java_thread_with_tid()' would be better.
> It'd better to list parameters in the opposite order.
>
> The call to is_valid_java_thread() is confusing:
>     627 } else if (java_thread != NULL && is_valid_java_thread(java_tid, java_thread)) {
>
> Why would the call ThreadTable::find_thread(java_tid) return a JavaThread with an unmatched java_tid?
>
>   
> Thanks,
> Serguei
>
> On 6/28/19, 9:40 PM, "David Holmes" <david.holmes at oracle.com> wrote:
>
>      Hi Daniil,
>      
>      The definition and use of this hashtable (yet another hashtable
>      implementation!) will need careful examination. We have to be concerned
>      about the cost of maintaining it when it may never even be queried. You
>      would need to look at footprint cost and performance impact.
>      
>      Unfortunately I'm just about to board a plane and will be out for the
>      next few days. I will try to look at this asap next week, but we will
>      need a lot more data on it.
>      
>      Thanks,
>      David
>
> On 6/28/19 3:31 PM, Daniil Titov wrote:
> Please review the change that improves performance of ThreadMXBean MXBean methods returning the
> information for specific threads. The change introduces the thread table that uses ConcurrentHashTable
> to store one-to-one the mapping between the thread ids and JavaThread objects and replaces the linear
> search over the thread list in ThreadsList::find_JavaThread_from_java_tid(jlong tid) method with the lookup
> in the thread table.
>
> Testing: Mach5 tier1,tier2 and tier3 tests successfully passed.
>
> Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
>
> Thanks!
>
> Best regards,
> Daniil
>
>
>
>
>
>
>