[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
From:       Daniil Titov <daniil.x.titov () oracle ! com>
Date:       2019-06-29 16:06:57
Message-ID: 6E7B043A-4647-4931-977C-1854CA7EBEC1 () oracle ! com
[Download RAW message or body]

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. 

Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8185005

Thanks!
--Daniil

From: <serguei.spitsyn@oracle.com>
Organization: Oracle Corporation
Date: Friday, June 28, 2019 at 7:56 PM
To: Daniil Titov <daniil.x.titov@oracle.com>, OpenJDK Serviceability \
<serviceability-dev@openjdk.java.net>, "hotspot-runtime-dev@openjdk.java.net" \
<hotspot-runtime-dev@openjdk.java.net>, "jmx-dev@openjdk.java.net" \
                <jmx-dev@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@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


[prev in list] [next in list] [prev in thread] [next in thread]