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.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
