[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
From: Daniil Titov <daniil.x.titov () oracle ! com>
Date: 2019-07-29 15:37:25
Message-ID: F097F031-D200-4AEA-887A-8DE18754483F () oracle ! com
[Download RAW message or body]
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@oracle.com" <serguei.spitsyn@oracle.com>
Date: Monday, July 29, 2019 at 2:12 AM
To: Daniil Titov <daniil.x.titov@oracle.com>, <daniel.daugherty@oracle.com>, OpenJDK \
Serviceability <serviceability-dev@openjdk.java.net>, \
"hotspot-runtime-dev@openjdk.java.net" <hotspot-runtime-dev@openjdk.java.net>, \
"jmx-dev@openjdk.java.net" <jmx-dev@openjdk.java.net>, David Holmes \
<david.holmes@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@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@oracle.com>
> Organization: Oracle Corporation
> Date: Friday, June 28, 2019 at 7:56 PM
> To: Daniil Titov <daniil.x.titov@oracle.com>, OpenJDK Serviceability \
<serviceability-dev@openjdk.java.net>, "hotspot-runtime-dev@openjdk.java.net" \
<hotspot-runtime-dev@openjdk.java.net>, "jmx-dev@openjdk.java.net" \
<jmx-dev@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@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
>
>
>
>
>
>
>
[Attachment #3 (text/html)]
<html xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type \
content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 15 \
(filtered medium)"><style><!-- /* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:11.0pt;
font-family:"Calibri",sans-serif;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:"Consolas",serif;}
span.new
{mso-style-name:new;}
span.EmailStyle21
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style></head><body lang=EN-US link=blue vlink=purple><div class=WordSection1><p \
class=MsoNormal>Hi Serguei,<o:p></o:p></p><p class=MsoNormal><o:p> </o:p></p><p \
class=MsoNormal>Thank you for catching this! <o:p></o:p></p><p \
class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>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.<o:p></o:p></p><p \
class=MsoNormal><o:p> </o:p></p><p class=MsoNormal>Thanks,<o:p></o:p></p><p \
class=MsoNormal>Daniil<o:p></o:p></p><p class=MsoNormal><o:p> </o:p></p><div \
style='border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in'><p \
class=MsoNormal><b><span style='font-size:12.0pt;color:black'>From: </span></b><span \
style='font-size:12.0pt;color:black'>"serguei.spitsyn@oracle.com" \
<serguei.spitsyn@oracle.com><br><b>Date: </b>Monday, July 29, 2019 at 2:12 \
AM<br><b>To: </b>Daniil Titov <daniil.x.titov@oracle.com>, \
<daniel.daugherty@oracle.com>, OpenJDK Serviceability \
<serviceability-dev@openjdk.java.net>, \
"hotspot-runtime-dev@openjdk.java.net" \
<hotspot-runtime-dev@openjdk.java.net>, "jmx-dev@openjdk.java.net" \
<jmx-dev@openjdk.java.net>, David Holmes \
<david.holmes@oracle.com><br><b>Subject: </b>Re: RFR: 8185005: Improve \
performance of ThreadMXBean.getThreadInfo(long ids[], int \
maxDepth)<o:p></o:p></span></p></div><div><p \
class=MsoNormal><o:p> </o:p></p></div><div><p class=MsoNormal \
style='margin-bottom:12.0pt'>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 \
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><o:p></o:p></p><pre> \
} else {<o:p></o:p></pre><pre> // reset contention statistics for a \
given thread<o:p></o:p></pre><pre><span class=new>+ JavaThread* \
java_thread = NULL;</span><o:p></o:p></pre><pre><span class=new>+ if \
(THREAD->is_Java_thread()) {</span><o:p></o:p></pre><pre><span class=new>+ \
JavaThread* current_thread = (JavaThread*)THREAD;</span><o:p></o:p></pre><pre><span \
class=new>+ if (tid == \
java_lang_Thread::thread_id(current_thread->threadObj())) \
{</span><o:p></o:p></pre><pre><span class=new>+ java_thread = \
current_thread;</span><o:p></o:p></pre><pre><span class=new>+ \
}</span><o:p></o:p></pre><pre><span class=new>+ \
}</span><o:p></o:p></pre><pre><span class=new>+ if (java_thread == \
NULL) {</span><o:p></o:p></pre><pre> JavaThread* java_thread = \
jtiwh.list()->find_JavaThread_from_java_tid(tid);<o:p></o:p></pre><pre><span \
class=new>+ }</span><o:p></o:p></pre><pre> if \
(java_thread == NULL) {<o:p></o:p></pre><pre> return \
false;<o:p></o:p></pre><pre> }<o:p></o:p></pre><p \
class=MsoNormal>Now, the definition of java_thread below:<o:p></o:p></p><pre><span \
class=new>+ if (java_thread == NULL) {</span><o:p></o:p></pre><pre> \
JavaThread* java_thread = \
jtiwh.list()->find_JavaThread_from_java_tid(tid);<o:p></o:p></pre><pre><span \
class=new>+ }</span><o:p></o:p></pre><p class=MsoNormal><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.</span><br> <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:<o:p></o:p></p></div><blockquote \
style='margin-top:5.0pt;margin-bottom:5.0pt'><pre>Hi David, Daniel, and \
Serguei,<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>Please review the new \
version of the fix, that makes the thread table initialization on demand and \
<o:p></o:p></pre><pre>moves it inside ThreadsList::find_JavaThread_from_java_tid(). \
At the creation time the thread table<o:p></o:p></pre><pre> is initialized with the \
threads from the current thread list. We don't want to hold Threads_lock \
<o:p></o:p></pre><pre>inside find_JavaThread_from_java_tid(), thus new threads \
still could be created while the thread<o:p></o:p></pre><pre>table is being \
initialized . Such threads will be found by the linear search and added to the thread \
table<o:p></o:p></pre><pre>later, in \
ThreadsList::find_JavaThread_from_java_tid().<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>The \
change also includes additional optimization for some callers of \
find_JavaThread_from_java_tid()<o:p></o:p></pre><pre>as Daniel suggested. \
<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>That is correct that \
ResolvedMethodTable was used as a blueprint for the thread table, however, I \
tried<o:p></o:p></pre><pre>to strip it of the all functionality that is not required \
in the thread table case.<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>We need to \
have the thread table resizable and allow it to grow as the number of threads \
increases to avoid <o:p></o:p></pre><pre>reserving excessive memory a-priori or \
deteriorating lookup times. The ServiceThread is responsible \
for<o:p></o:p></pre><pre>growing the thread table when \
required.<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>There is no \
ConcurrentHashTable available in Java 8 and for backporting this fix to Java 8 \
another implementation <o:p></o:p></pre><pre>of the hash table, probably originally \
suggested in the patch attached to the JBS issue, should be used. It will \
make<o:p></o:p></pre><pre>the backporting more complicated, however, adding a new \
Implementation of the hash table in Java 14 while it <o:p></o:p></pre><pre>already \
has ConcurrentHashTable doesn't seem reasonable for \
me.<o:p></o:p></pre><pre><o:p> </o:p></pre><pre><o:p> </o:p></pre><pre>Webrev: \
<a href="http://cr.openjdk.java.net/~dtitov/8185005/webrev.03">http://cr.openjdk.java.net/~dtitov/8185005/webrev.03</a><o:p></o:p></pre><pre>Bug: \
<a href="https://bugs.openjdk.java.net/browse/JDK-8185005">https://bugs.openjdk.java.net/browse/JDK-8185005</a> \
<o:p></o:p></pre><pre><o:p> </o:p></pre><pre>Thanks!<o:p></o:p></pre><pre>--Danii \
l<o:p></o:p></pre><pre><o:p> </o:p></pre><pre><o:p> </o:p></pre><pre>On \
7/8/19, 3:24 PM, "Daniel D. Daugherty" <a \
href="mailto:daniel.daugherty@oracle.com"><daniel.daugherty@oracle.com></a> \
wrote:<o:p></o:p></pre><pre><o:p> </o:p></pre><pre> On 6/29/19 12:06 PM, \
Daniil Titov wrote:<o:p></o:p></pre><pre> > Hi Serguei and \
David,<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > Serguei is \
right, ThreadTable::find_thread(java_tid) cannot return a JavaThread with an \
unmatched java_tid.<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > \
Please find a new version of the fix that includes the changes Serguei \
suggested.<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > \
Regarding the concern about the maintaining the thread table when it may never even \
be queried, one of<o:p></o:p></pre><pre> > the options could be to add \
ThreadTable ::isEnabled flag, set it to "false" by default, and wrap the \
calls to the thread table<o:p></o:p></pre><pre> > in ThreadsSMRSupport \
add_thread() and remove_thread() methods to check this flag.<o:p></o:p></pre><pre> \
><o:p></o:p></pre><pre> > When \
ThreadsList::find_JavaThread_from_java_tid() is called for the first time it could \
check if ThreadTable ::isEnabled<o:p></o:p></pre><pre> > Is on and if not \
then set it on and populate the thread table with all existing threads from the \
thread list.<o:p></o:p></pre><pre> <o:p></o:p></pre><pre> I have the \
same concerns as David H. about this new ThreadTable.<o:p></o:p></pre><pre> \
ThreadsList::find_JavaThread_from_java_tid() is only called from \
code<o:p></o:p></pre><pre> in src/hotspot/share/services/management.cpp so I \
think that table<o:p></o:p></pre><pre> needs to enabled and populated only if \
it is going to be used.<o:p></o:p></pre><pre> <o:p></o:p></pre><pre> \
I've taken a look at the webrev below and I see that David has<o:p></o:p></pre><pre> \
followed up with additional comments. Before I do a crawl \
through<o:p></o:p></pre><pre> code review for this, I would like to see the \
ThreadTable stuff<o:p></o:p></pre><pre> made optional and David's other \
comments addressed.<o:p></o:p></pre><pre> <o:p></o:p></pre><pre> Another \
possible optimization is for callers of<o:p></o:p></pre><pre> \
find_JavaThread_from_java_tid() to save the calling thread's<o:p></o:p></pre><pre> \
tid value before they loop and if the current tid == saved_tid<o:p></o:p></pre><pre> \
then use the current JavaThread* instead of calling<o:p></o:p></pre><pre> \
find_JavaThread_from_java_tid() to get the JavaThread*.<o:p></o:p></pre><pre> \
<o:p></o:p></pre><pre> Dan<o:p></o:p></pre><pre> <o:p></o:p></pre><pre> \
><o:p></o:p></pre><pre> > Webrev: <a \
href="https://cr.openjdk.java.net/~dtitov/8185005/webrev.02/">https://cr.openjdk.java.net/~dtitov/8185005/webrev.02/</a><o:p></o:p></pre><pre> \
> Bug: <a href="https://bugs.openjdk.java.net/browse/JDK-8185005">https://bugs.openjdk.java.net/browse/JDK-8185005</a><o:p></o:p></pre><pre> \
><o:p></o:p></pre><pre> > Thanks!<o:p></o:p></pre><pre> > \
--Daniil<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > From: <a \
href="mailto:serguei.spitsyn@oracle.com"><serguei.spitsyn@oracle.com></a><o:p></o:p></pre><pre> \
> Organization: Oracle Corporation<o:p></o:p></pre><pre> > Date: Friday, \
June 28, 2019 at 7:56 PM<o:p></o:p></pre><pre> > To: Daniil Titov <a \
href="mailto:daniil.x.titov@oracle.com"><daniil.x.titov@oracle.com></a>, \
OpenJDK Serviceability <a \
href="mailto:serviceability-dev@openjdk.java.net"><serviceability-dev@openjdk.java.net></a>, \
<a href="mailto:hotspot-runtime-dev@openjdk.java.net">"hotspot-runtime-dev@openjdk.java.net"</a> \
<a href="mailto:hotspot-runtime-dev@openjdk.java.net"><hotspot-runtime-dev@openjdk.java.net></a>, \
<a href="mailto:jmx-dev@openjdk.java.net">"jmx-dev@openjdk.java.net"</a> <a \
href="mailto:jmx-dev@openjdk.java.net"><jmx-dev@openjdk.java.net></a><o:p></o:p></pre><pre> \
> Subject: Re: RFR: 8185005: Improve performance of \
ThreadMXBean.getThreadInfo(long ids[], int maxDepth)<o:p></o:p></pre><pre> \
><o:p></o:p></pre><pre> > Hi Daniil,<o:p></o:p></pre><pre> \
><o:p></o:p></pre><pre> > I have several quick \
comments.<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > The \
indent in the hotspot c/c++ files has to be 2, not 4.<o:p></o:p></pre><pre> \
><o:p></o:p></pre><pre> > <a \
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><o:p></o:p></pre><pre> \
> 614 JavaThread* ThreadsList::find_JavaThread_from_java_tid(jlong java_tid) const \
{<o:p></o:p></pre><pre> > 615 JavaThread* java_thread = \
ThreadTable::find_thread(java_tid);<o:p></o:p></pre><pre> > 616 \
if (java_thread == NULL && java_tid == PMIMORDIAL_JAVA_TID) \
{<o:p></o:p></pre><pre> > 617 // \
ThreadsSMRSupport::add_thread() is not called for the \
primordial<o:p></o:p></pre><pre> > 618 // thread. Thus, \
we find this thread with a linear search and add it<o:p></o:p></pre><pre> > \
619 // to the thread table.<o:p></o:p></pre><pre> > 620 \
for (uint i = 0; i < length(); i++) {<o:p></o:p></pre><pre> > 621 \
JavaThread* thread = thread_at(i);<o:p></o:p></pre><pre> > 622 \
if (is_valid_java_thread(java_tid,thread)) {<o:p></o:p></pre><pre> > 623 \
ThreadTable::add_thread(java_tid, thread);<o:p></o:p></pre><pre> > 624 \
return thread;<o:p></o:p></pre><pre> > 625 \
}<o:p></o:p></pre><pre> > 626 }<o:p></o:p></pre><pre> \
> 627 } else if (java_thread != NULL && \
is_valid_java_thread(java_tid, java_thread)) {<o:p></o:p></pre><pre> > \
628 return java_thread;<o:p></o:p></pre><pre> > 629 \
}<o:p></o:p></pre><pre> > 630 return NULL;<o:p></o:p></pre><pre> \
> 631 }<o:p></o:p></pre><pre> > 632 bool \
ThreadsList::is_valid_java_thread(jlong java_tid, JavaThread* java_thread) \
{<o:p></o:p></pre><pre> > 633 oop tobj = \
java_thread->threadObj();<o:p></o:p></pre><pre> > 634 // \
Ignore the thread if it hasn't run yet, has exited<o:p></o:p></pre><pre> > \
635 // or is starting to exit.<o:p></o:p></pre><pre> > 636 \
return (tobj != NULL && !java_thread->is_exiting() \
&&<o:p></o:p></pre><pre> > 637 java_tid \
== java_lang_Thread::thread_id(tobj));<o:p></o:p></pre><pre> > 638 \
}<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > 615 \
JavaThread* java_thread = ThreadTable::find_thread(java_tid);<o:p></o:p></pre><pre> \
><o:p></o:p></pre><pre> > I'd suggest to rename find_thread() to \
find_thread_by_tid().<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> \
> A space is missed after the comma:<o:p></o:p></pre><pre> > 622 if \
(is_valid_java_thread(java_tid,thread)) {<o:p></o:p></pre><pre> \
><o:p></o:p></pre><pre> > An empty line is needed before \
L632.<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > The name \
'is_valid_java_thread' looks wrong (or confusing) to me.<o:p></o:p></pre><pre> \
> Something like 'is_alive_java_thread_with_tid()' would be \
better.<o:p></o:p></pre><pre> > It'd better to list parameters in the \
opposite order.<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > The \
call to is_valid_java_thread() is confusing:<o:p></o:p></pre><pre> > \
627 } else if (java_thread != NULL && is_valid_java_thread(java_tid, \
java_thread)) {<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > Why \
would the call ThreadTable::find_thread(java_tid) return a JavaThread with an \
unmatched java_tid?<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > \
<o:p></o:p></pre><pre> > Thanks,<o:p></o:p></pre><pre> > \
Serguei<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > On 6/28/19, \
9:40 PM, "David Holmes" <a \
href="mailto:david.holmes@oracle.com"><david.holmes@oracle.com></a> \
wrote:<o:p></o:p></pre><pre> ><o:p></o:p></pre><pre> > Hi \
Daniil,<o:p></o:p></pre><pre> > <o:p></o:p></pre><pre> > \
The definition and use of this hashtable (yet another hashtable<o:p></o:p></pre><pre> \
> implementation!) will need careful examination. We have to be \
concerned<o:p></o:p></pre><pre> > about the cost of maintaining it \
when it may never even be queried. You<o:p></o:p></pre><pre> > \
would need to look at footprint cost and performance impact.<o:p></o:p></pre><pre> \
> <o:p></o:p></pre><pre> > Unfortunately I'm just \
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic