[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: "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date: 2019-07-29 9:12:01
Message-ID: 65e52a11-21b3-58d2-a707-c7ecdc8bcff7 () oracle ! com
[Download RAW message or body]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Daniil,<br>
<br>
Probably, it'd make sense to re-iterate on this after you resolve
David's comments.<br>
Now, just one comment though as it looks dangerous.<br>
<br>
<a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dtitov/8185005/webrev.03/src/hotspot/share/services/ \
management.cpp.udiff.html">http://cr.openjdk.java.net/~dtitov/8185005/webrev.03/src/hotspot/share/services/management.cpp.udiff.html</a><br>
<br>
<pre> } else {
// reset contention statistics for a given thread
<span class="new">+ JavaThread* java_thread = NULL;</span>
<span class="new">+ if (THREAD->is_Java_thread()) {</span>
<span class="new">+ JavaThread* current_thread = (JavaThread*)THREAD;</span>
<span class="new">+ if (tid == \
java_lang_Thread::thread_id(current_thread->threadObj())) {</span> <span \
class="new">+ java_thread = current_thread;</span> <span class="new">+ \
}</span> <span class="new">+ }</span>
<span class="new">+ if (java_thread == NULL) {</span>
JavaThread* java_thread = \
jtiwh.list()->find_JavaThread_from_java_tid(tid); <span class="new">+ \
}</span> if (java_thread == NULL) {
return false;
}
</pre>
Now, the definition of java_thread
below:<br>
<pre><span class="new">+ if (java_thread == NULL) {</span>
JavaThread* java_thread = \
jtiwh.list()->find_JavaThread_from_java_tid(tid); <span class="new">+ \
}</span></pre> <br>
becomes completely useless because the block of its definition is
ended right away.<br>
Here, overriding the local variable <span class="new">'java_thread'
does not look as a good idea.<br>
</span> <br>
Then how was it really tested?<br>
<br>
Thanks,<br>
Serguei<br>
<br>
<br>
<br>
<br>
<br>
On 7/24/19 10:21, Daniil Titov wrote:<br>
</div>
<blockquote type="cite"
cite="mid:76BCC96D-DB5D-409A-95D5-3A64B893832D@oracle.com">
<pre class="moz-quote-pre" wrap="">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: <a class="moz-txt-link-freetext" \
href="http://cr.openjdk.java.net/~dtitov/8185005/webrev.03">http://cr.openjdk.java.net/~dtitov/8185005/webrev.03</a>
Bug: <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8185005">https://bugs.openjdk.java.net/browse/JDK-8185005</a> \
Thanks!
--Daniil
On 7/8/19, 3:24 PM, "Daniel D. Daugherty" <a class="moz-txt-link-rfc2396E" \
href="mailto:daniel.daugherty@oracle.com"><daniel.daugherty@oracle.com></a> \
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: <a class="moz-txt-link-freetext" \
href="https://cr.openjdk.java.net/~dtitov/8185005/webrev.02/">https://cr.openjdk.java.net/~dtitov/8185005/webrev.02/</a>
> Bug: <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8185005">https://bugs.openjdk.java.net/browse/JDK-8185005</a>
>
> Thanks!
> --Daniil
>
> From: <a class="moz-txt-link-rfc2396E" \
href="mailto:serguei.spitsyn@oracle.com"><serguei.spitsyn@oracle.com></a> > \
Organization: Oracle Corporation > Date: Friday, June 28, 2019 at 7:56 PM
> To: Daniil Titov <a class="moz-txt-link-rfc2396E" \
href="mailto:daniil.x.titov@oracle.com"><daniil.x.titov@oracle.com></a>, \
OpenJDK Serviceability <a class="moz-txt-link-rfc2396E" \
href="mailto:serviceability-dev@openjdk.java.net"><serviceability-dev@openjdk.java.net></a>, \
<a class="moz-txt-link-rfc2396E" \
href="mailto:hotspot-runtime-dev@openjdk.java.net">"hotspot-runtime-dev@openjdk.java.net"</a> \
<a class="moz-txt-link-rfc2396E" \
href="mailto:hotspot-runtime-dev@openjdk.java.net"><hotspot-runtime-dev@openjdk.java.net></a>, \
<a class="moz-txt-link-rfc2396E" \
href="mailto:jmx-dev@openjdk.java.net">"jmx-dev@openjdk.java.net"</a> <a \
class="moz-txt-link-rfc2396E" \
href="mailto:jmx-dev@openjdk.java.net"><jmx-dev@openjdk.java.net></a> > \
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.
>
> <a class="moz-txt-link-freetext" \
href="https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/src/hotspot/share/runtime/ \
threadSMR.cpp.frames.html">https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/src/hotspot/share/runtime/threadSMR.cpp.frames.html</a>
> 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" <a class="moz-txt-link-rfc2396E" \
href="mailto:david.holmes@oracle.com"><david.holmes@oracle.com></a> 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: <a class="moz-txt-link-freetext" \
href="https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/">https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/</a>
> Bug: <a class="moz-txt-link-freetext" \
href="https://bugs.openjdk.java.net/browse/JDK-8185005">https://bugs.openjdk.java.net/browse/JDK-8185005</a>
>
> Thanks!
>
> Best regards,
> Daniil
>
>
>
>
>
>
>
</pre>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic