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