8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
Daniil Titov
daniil.x.titov at oracle.com
Mon Jul 29 15:37:25 UTC 2019
Hi Serguei,
Thank you for catching this!
Regarding the testing, at this stage, I run all tier1- tier3 tests . I will check why this specific case was not included in the tests and will add it if required.
Thanks,
Daniil
From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
Date: Monday, July 29, 2019 at 2:12 AM
To: Daniil Titov <daniil.x.titov at oracle.com>, <daniel.daugherty 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>, David Holmes <david.holmes at oracle.com>
Subject: Re: RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
Hi Daniil,
Probably, it'd make sense to re-iterate on this after you resolve David's comments.
Now, just one comment though as it looks dangerous.
http://cr.openjdk.java.net/~dtitov/8185005/webrev.03/src/hotspot/share/services/management.cpp.udiff.html
} else {
// reset contention statistics for a given thread
+ JavaThread* java_thread = NULL;
+ if (THREAD->is_Java_thread()) {
+ JavaThread* current_thread = (JavaThread*)THREAD;
+ if (tid == java_lang_Thread::thread_id(current_thread->threadObj())) {
+ java_thread = current_thread;
+ }
+ }
+ if (java_thread == NULL) {
JavaThread* java_thread = jtiwh.list()->find_JavaThread_from_java_tid(tid);
+ }
if (java_thread == NULL) {
return false;
}
Now, the definition of java_thread below:
+ if (java_thread == NULL) {
JavaThread* java_thread = jtiwh.list()->find_JavaThread_from_java_tid(tid);
+ }
becomes completely useless because the block of its definition is ended right away.
Here, overriding the local variable 'java_thread' does not look as a good idea.
Then how was it really tested?
Thanks,
Serguei
On 7/24/19 10:21, Daniil Titov wrote:
Hi David, Daniel, and Serguei,
Please review the new version of the fix, that makes the thread table initialization on demand and
moves it inside ThreadsList::find_JavaThread_from_java_tid(). At the creation time the thread table
is initialized with the threads from the current thread list. We don't want to hold Threads_lock
inside find_JavaThread_from_java_tid(), thus new threads still could be created while the thread
table is being initialized . Such threads will be found by the linear search and added to the thread table
later, in ThreadsList::find_JavaThread_from_java_tid().
The change also includes additional optimization for some callers of find_JavaThread_from_java_tid()
as Daniel suggested.
That is correct that ResolvedMethodTable was used as a blueprint for the thread table, however, I tried
to strip it of the all functionality that is not required in the thread table case.
We need to have the thread table resizable and allow it to grow as the number of threads increases to avoid
reserving excessive memory a-priori or deteriorating lookup times. The ServiceThread is responsible for
growing the thread table when required.
There is no ConcurrentHashTable available in Java 8 and for backporting this fix to Java 8 another implementation
of the hash table, probably originally suggested in the patch attached to the JBS issue, should be used. It will make
the backporting more complicated, however, adding a new Implementation of the hash table in Java 14 while it
already has ConcurrentHashTable doesn't seem reasonable for me.
Webrev: http://cr.openjdk.java.net/~dtitov/8185005/webrev.03
Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
Thanks!
--Daniil
On 7/8/19, 3:24 PM, "Daniel D. Daugherty" <daniel.daugherty at oracle.com> wrote:
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 hotspot-runtime-dev
mailing list