RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Mon Jul 8 22:24:18 UTC 2019
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
>
>
>
>
>
>
>
More information about the serviceability-dev
mailing list