Hi Serguei,
void ThreadTable::lazy_initialize(const ThreadsList *threads) {
if (_is_initialized) {
return;
> }
MutexLocker ml(ThreadTableCreate_lock);
If I understood you correctly in the code snippet you sent you meant
to use
Threads_lock, not ThreadTableCreate_lock, right?
The original idea was to do a minimal amount of work while holding
the lock and
hold the lock for as short period of time as possible to not block
other threads when it is not necessary.
With the suggested approach no new threads could be started until the
thread table is created and
populated with all threads running inside a Java application and in
case of large app there could be
thousands of them.
And if we try to use 2 locks, ThreadTableCreate_lock as in your
snippet and then the nested Threads_lock around
thread->is_exiting() and add_thread(java_tid, thread) lines then it
will not work since the rank of Threads_lock
is higher than the rank of ThreadTableCreate_lock.
So choosing between blocking new threads from starting and
potentially allowing
some other monitoring thread to do a one-time linear scan I think it
makes sense to choose the latter.
Thanks!
Best regards,
Daniil
From: "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
Date: Thursday, September 19, 2019 at 10:30 PM
To: Daniil Titov <daniil.x.ti...@oracle.com>, Robbin Ehn
<robbin....@oracle.com>, David Holmes <david.hol...@oracle.com>,
<daniel.daughe...@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>, Claes Redestad <claes.redes...@oracle.com>
Subject: Re: RFR: 8185005: Improve performance of
ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
Hi Daniil,
I think, it is better to grab the thread_lock just once at lazy
initialization.
It would look simpler, something, like this would work:
void ThreadTable::lazy_initialize(const ThreadsList *threads) {
if (_is_initialized) {
return;
}
MutexLocker ml(ThreadTableCreate_lock);
if (_is_initialized) {
// There is no obvious benefits in allowing the thread table
// being concurrently populated during the initalization.
return;
}
create_table(threads->length());
_is_initialized = true;
for (uint i = 0; i < threads->length(); i++) {
JavaThread* thread = threads->thread_at(i);
oop tobj = thread->threadObj();
if (tobj != NULL) {
jlong java_tid = java_lang_Thread::thread_id(tobj);
if (!thread->is_exiting()) {
// Must be inside the lock to ensure that we don't add the
thread to the table
// that has just passed the removal point in
ThreadsSMRSupport::remove_thread()
add_thread(java_tid, thread);
}
}
}
}
Otherwise, concurrent executions of the find_JavaThread_from_java_tid()
will sometimes do a linear search of threads that are not included
yet to
the ThreadTable from the ThreadsList (which is used for lazy
initialization).
Instead, it is better to wait for the lazy_initialization() to complete.
Thanks,
Serguei
On 9/19/19 17:30, Daniil Titov wrote:
Hi David and Serguei,
Please review new version of the fix that includes the changes
Serguei suggested:
1. If racing threads initialize the thread table only one of these
threads will populate the table with the threads from the thread list
2. The code that adds the thread to the tread table is put inside
Threads_lock to ensure that we cannot accidentally add the thread
that has just passed the removal point in
ThreadsSMRSupport::remove_thread()
The changes are in ThreadTable::lazy_initialize() method only.
Testing: Mach5 tier1, tier2, tier3, tier4, and tier5 tests
successfully passed.
Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.07/
Bug : https://bugs.openjdk.java.net/browse/JDK-8185005
Thank you!
--Daniil
On 9/18/19, 1:01 AM, mailto:serguei.spit...@oracle.com
mailto:serguei.spit...@oracle.com wrote:
Hi Daniil,
On 9/17/19 17:13, Daniil Titov wrote:
> Hi Serguei,
>
> Please find below my answers to the concerns you mentioned in
the previous email.
>
> 1.
> > I have a concern about the checks for thread->is_exiting().
> > - the lines 632-633 are useless as they do not really
protect from returning an exiting thread
>> It is interesting what might happen if an exiting thread is
returned by the
>> ThreadsList::find_JavaThread_from_java_tid ().
>> Does it make sense to develop a test that would cover these
cases?
> I agree, it doesn't really provide any protection so it makes
sense just remove it.
Now, I'm not that confident about it. :)
> The current implementation
> find_JavaThread_from_java_tid() doesn't provide such
protection as well, since the thread could start exiting
> immediately after method find_JavaThread_from_java_tid()
returns, so the assumption is that the callers of
> find_JavaThread_from_java_tid() are expecting to deal with
such threads and looking on some of them shows that
> they usually try to retrieve threadObj or a thread statistic
object and if it is NULL that just do nothing.
If I understand it correctly, the jt->threadObj() can
remain non-NULL
for some time while jt->is_exiting() == true.
It is not clear how reliable is to use it.
But this is a pre-existing issue. It is not you who introduced
it. :)
So, we can skip it for now.
But for the record, we may have a source of intermittent issues.
> I'm not sure we could cover this specific case with the
test. The window between find_JavaThread_from_java_tid() returns and
the caller
> continues the execution is too small. The window between the
thread started exiting and removed itself from the thread table is
very small as well.
Understand.
> 2.
>> - the lines 105-108 can result in adding exiting threads
into the ThreadTable
> I agree, it was missed, we need to wrap this code inside
Thread_lock in the similar way as it is done
find_JavaThread_from_java_tid()
Okay, thanks!
> 3.
>> I would suggest to rewrite this fragment in a safe way:
>> 95 {
>> 96 MutexLocker ml(ThreadTableCreate_lock);
>> 97 if (!_is_initialized) {
>> 98 create_table(threads->length());
>> 99 _is_initialized = true;
>> 100 }
>> 101 }
>> as:
>> {
>> MutexLocker ml(ThreadTableCreate_lock);
>> if (_is_initialized) {
>> return;
> > }
> > create_table(threads->length());
> > _is_initialized = true;
> > }
>
> It was an intension to not block while populating the table
with the threads from the current thread list.
> There is no needs to have other threads that call
find_JavaThread_from_java_tid() be blocked and waiting for
> it to complete since the requested thread could be not
present in the thread list that triggers the thread table
> initialization. Plus in case of racing initialization it
allows threads from not original thread lists be added to the table
> and thus avoid the linear scan when these thread are looked up
for the first time.
I've replied to David in another email.
Let's talk once more about it tomorrow.
> 4.
>>> The case you have described is exact the reason why we still
have a code inside
>>> ThreadsList::find_JavaThread_from_java_tid() method that
does a linear scan and adds
>>> the requested thread to the thread table if it is not
there ( lines 614-613 below).
>> I disagree because it is easy to avoid concurrent ThreadTable
>> initialization (please, see my separate email).
>> The reason for this code is to cover a case of late/lazy
ThreadTable
>> initialization.
> David Holmes replied to this in a separate email providing a
very detailed
> explanation of the possible cases and how the proposed
implementation satisfies them.
Yes. Please, see above.
Thanks,
Serguei
> Best regards,
> Daniil
>
> From: mailto:serguei.spit...@oracle.com
mailto:serguei.spit...@oracle.com
> Date: Tuesday, September 17, 2019 at 1:53 AM
> To: Daniil Titov mailto:daniil.x.ti...@oracle.com, Robbin Ehn
mailto:robbin....@oracle.com, David Holmes
mailto:david.hol...@oracle.com, mailto:daniel.daughe...@oracle.com,
OpenJDK Serviceability mailto:serviceability-dev@openjdk.java.net,
mailto:hotspot-runtime-...@openjdk.java.net
mailto:hotspot-runtime-...@openjdk.java.net,
mailto:jmx-...@openjdk.java.net mailto:jmx-...@openjdk.java.net,
Claes Redestad mailto:claes.redes...@oracle.com
> Subject: Re: RFR: 8185005: Improve performance of
ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
>
> Hi Daniil,
>
> Thank you for you patience in working on this issue!
> Also, I like that the current thread related optimizations in
management.cpp were factored out.
> It was a good idea to separate them.
>
> I have a concern about the checks for thread->is_exiting().
> The threads are added to and removed from the ThreadTable
under protection of Threads_lock.
> However, the thread->is_exiting() checks are not protected,
and so, they are racy.
>
> There is a couple of such checks to mention:
> 611 JavaThread*
ThreadsList::find_JavaThread_from_java_tid(jlong java_tid) const {
> 612 ThreadTable::lazy_initialize(this);
> 613 JavaThread* thread =
ThreadTable::find_thread_by_tid(java_tid);
> 614 if (thread == NULL) {
> 615 // If the thread is not found in the table find it
> 616 // with a linear search and add to the table.
> 617 for (uint i = 0; i < length(); i++) {
> 618 thread = thread_at(i);
> 619 oop tobj = thread->threadObj();
> 620 // Ignore the thread if it hasn't run yet, has exited
> 621 // or is starting to exit.
> 622 if (tobj != NULL && java_tid ==
java_lang_Thread::thread_id(tobj)) {
> 623 MutexLocker ml(Threads_lock);
> 624 // Must be inside the lock to ensure that we
don't add the thread to the table
> 625 // that has just passed the removal point in
ThreadsSMRSupport::remove_thread()
> 626 if (!thread->is_exiting()) {
> 627 ThreadTable::add_thread(java_tid, thread);
> 628 return thread;
> 629 }
> 630 }
> 631 }
> 632 } else if (!thread->is_exiting()) {
> 633 return thread;
> 634 }
> 635 return NULL;
> 636 }
> ...
> 93 void ThreadTable::lazy_initialize(const ThreadsList
*threads) {
> 94 if (!_is_initialized) {
> 95 {
> 96 MutexLocker ml(ThreadTableCreate_lock);
> 97 if (!_is_initialized) {
> 98 create_table(threads->length());
> 99 _is_initialized = true;
> 100 }
> 101 }
> 102 for (uint i = 0; i < threads->length(); i++) {
> 103 JavaThread* thread = threads->thread_at(i);
> 104 oop tobj = thread->threadObj();
> 105 if (tobj != NULL && !thread->is_exiting()) {
> 106 jlong java_tid = java_lang_Thread::thread_id(tobj);
> 107 add_thread(java_tid, thread);
> 108 }
> 109 }
> 110 }
> 111 }
>
> A thread may start exiting right after the checks at the lines
626 and 105.
> So that:
> - the lines 632-633 are useless as they do not really
protect from returning an exiting thread
> - the lines 105-108 can result in adding exiting threads
into the ThreadTable
>
> Please, note, the lines 626-629 are safe in terms of addition
to the ThreadTable as they
> are protected with the Threads_lock. But the returned thread
still can exit after that.
> It is interesting what might happen if an exiting thread is
returned by the
> ThreadsList::find_JavaThread_from_java_tid ().
>
> Does it make sense to develop a test that would cover these
cases?
>
> Thanks,
> Serguei
>
>
> On 9/16/19 11:18, Daniil Titov wrote:
> Hello,
>
> After investigating with Claes the impact of this change on
the performance (thanks a lot Claes for helping with it!) the
conclusion was that the impact on the thread startup time is not a
blocker for this change.
>
> I also measured the memory footprint using Native Memory
Tracking and results showed around 40 bytes per live thread.
>
> Please review a new version of the fix, webrev.06 [1]. Just
to remind, webrev.05 was abandoned and webrev.06 [1] is webrev.04
[3] minus changes in src/hotspot/share/services/management.cpp (that
were factored out to a separate issue [4]) and plus a change in
ThreadsList::find_JavaThread_from_java_tid() method (please, see
below) that addresses the problem Robbin found and puts the code
that adds a new thread to the thread table inside Threads_lock.
>
> src/hotspot/share/runtime/threadSMR.cpp
>
> 622 if (tobj != NULL && java_tid ==
java_lang_Thread::thread_id(tobj)) {
> 623 MutexLocker ml(Threads_lock);
> 624 // Must be inside the lock to ensure that we don't
add the thread to the table
> 625 // that has just passed the removal point in
ThreadsSMRSupport::remove_thread()
> 626 if (!thread->is_exiting()) {
> 627 ThreadTable::add_thread(java_tid, thread);
> 628 return thread;
> 629 }
> 630 }
>
> [1] Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.06
> [2] Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
> [3] https://cr.openjdk.java.net/~dtitov/8185005/webrev.04
> [4] https://bugs.openjdk.java.net/browse/JDK-8229391
>
> Thank you,
> Daniil
>
>
>
> >
> > On 8/4/19, 7:54 PM, "David Holmes"
mailto:david.hol...@oracle.com wrote:
> >
> > Hi Daniil,
> >
> > On 3/08/2019 8:16 am, Daniil Titov wrote:
> > > Hi David,
> > >
> > > Thank you for your detailed review. Please
review a new version of the fix that includes
> > > the changes you suggested:
> > > - ThreadTableCreate_lock scope is reduced to
cover the creation of the table only;
> > > - ThreadTableCreate_lock is made
_safepoint_check_always;
> >
> > Okay.
> >
> > > - ServiceThread is no longer responsible for
the resizing of the thread table, instead,
> > > the thread table is changed to grow on
demand by the thread that is doing the addition;
> >
> > Okay - I'm happy to get the serviceThread out
of the picture here.
> >
> > > - fixed nits and formatting issues.
> >
> > Okay.
> >
> > >>> The change also includes additional
optimization for some callers of find_JavaThread_from_java_tid()
> > >>> as Daniel suggested.
> > >> Not sure it's best to combine these, but if
they are limited to the
> > >> changes in management.cpp only then that
may be okay.
> > >
> > > The additional optimization for some callers
of find_JavaThread_from_java_tid() is
> > > limited to management.cpp (plus a new test)
so I left them in the webrev but
> > > I also could move it in the separate issue
if required.
> >
> > I'd prefer this part of be separated out, but
won't insist. Let's see if
> > Dan or Serguei have a strong opinion.
> >
> > > > src/hotspot/share/runtime/threadSMR.cpp
> > > >755 jlong tid =
SharedRuntime::get_java_tid(thread);
> > > > 926 jlong tid =
SharedRuntime::get_java_tid(thread);
> > > > I think it cleaner/better to just use
> > > > jlong tid =
java_lang_Thread::thread_id(thread->threadObj());
> > > > as we know thread is not NULL, it is a
JavaThread and it has to have a
> > > > non-null threadObj.
> > >
> > > I had to leave this code unchanged since it
turned out the threadObj is null
> > > when VM is destroyed:
> > >
> > > V [libjvm.so+0xe165d7]
oopDesc::long_field(int) const+0x67
> > > V [libjvm.so+0x16e06c6]
ThreadsSMRSupport::add_thread(JavaThread*)+0x116
> > > V [libjvm.so+0x16d1302]
Threads::add(JavaThread*, bool)+0x82
> > > V [libjvm.so+0xef8369]
attach_current_thread.part.197+0xc9
> > > V [libjvm.so+0xec136c] jni_DestroyJavaVM+0x6c
> > > C [libjli.so+0x4333] JavaMain+0x2c3
> > > C [libjli.so+0x8159] ThreadJavaMain+0x9
> >
> > This is actually nothing to do with the VM
being destroyed, but is an
> > issue with JNI_AttachCurrentThread and its
interaction with the
> > ThreadSMR iterators. The attach process is:
> > - create JavaThread
> > - mark as "is attaching via jni"
> > - add to ThreadsList
> > - create java.lang.Thread object (you can only
execute Java code after
> > you are attached)
> > - mark as "attach completed"
> >
> > So while a thread "is attaching" it will be
seen by the ThreadSMR thread
> > iterator but will have a NULL java.lang.Thread
object.
> >
> > We special-case attaching threads in a number
of places in the VM and I
> > think we should be explicitly doing something
here to filter out
> > attaching threads, rather than just being
tolerant of a NULL j.l.Thread
> > object. Specifically in
ThreadsSMRSupport::add_thread:
> >
> > if (ThreadTable::is_initialized() &&
!thread->is_attaching_via_jni()) {
> > jlong tid =
java_lang_Thread::thread_id(thread->threadObj());
> > ThreadTable::add_thread(tid, thread);
> > }
> >
> > Note that in ThreadsSMRSupport::remove_thread
we can use the same guard,
> > which covers the case the JNI attach
encountered an error trying to
> > create the j.l.Thread object.
> >
> > >> src/hotspot/share/services/threadTable.cpp
> > >> 71 static uintx get_hash(Value const&
value, bool* is_dead) {
> > >
> > >> The is_dead parameter still bothers me
here. I can't make enough sense
> > >> out of the template code in
ConcurrentHashtable to see why we have to
> > >> have it, but I'm concerned that its very
existence means we perhaps
> > >> should not be trying to extend CHT in this
context. ??
> > >
> > > My understanding is that is_dead parameter
provides a mechanism for
> > > ConcurrentHashtable to remove stale entries
that were not explicitly
> > > removed by calling
ConcurrentHashTable::remove() method.
> > > I think that just because in our case we
don't use this mechanism doesn't
> > > mean we should not use ConcurrentHashTable.
> >
> > Can you confirm that this usage is okay with
Robbin Ehn please. He's
> > back from vacation this week.
> >
> > >> I would still want to see what impact this
has on thread
> > >> startup cost, both with and without the
table being initialized.
> > >
> > > I run a test that initializes the table by
calling ThreadMXBean.get getThreadInfo(),
> > > starts some threads as a worm-up, and then
creates and starts 100,000 threads
> > > (each thread just sleeps for 100 ms). In
case when the thread table is enabled
> > > 100,000 threads are created and started for
about 15200 ms. If the thread table
> > > is off the test takes about 14800 ms. Based
on this information the enabled
> > > thread table makes the thread startup about
2.7% slower.
> >
> > That doesn't sound very good. I think we may
need to Claes involved to
> > help investigate overall performance impact here.
> >
> > > Webrev:
https://cr.openjdk.java.net/~dtitov/8185005/webrev.04/
> > > Bug:
https://bugs.openjdk.java.net/browse/JDK-8185005
> >
> > No further code comments.
> >
> > I didn't look at the test in detail.
> >
> > Thanks,
> > David
> >
> > > Thanks!
> > > --Daniil
> > >
> > >
> > > On 7/29/19, 12:53 AM, "David Holmes"
mailto:david.hol...@oracle.com wrote:
> > >
> > > Hi Daniil,
> > >
> > > Overall I think this is a reasonable
approach but I would still like to
> > > see some performance and footprint
numbers, both to verify it fixes the
> > > problem reported, and that we are not
getting penalized elsewhere.
> > >
> > > On 25/07/2019 3:21 am, 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 initialization allows the created
but unpopulated, or partially
> > > populated, table to be seen by other
threads - is that your intention?
> > > It seems it should be okay as the other
threads will then race with the
> > > initializing thread to add specific
entries, and this is a concurrent
> > > map so that should be functionally
correct. But if so then I think you
> > > can also reduce the scope of the
ThreadTableCreate_lock so that it
> > > covers creation of the table only, not
the initial population of the table.
> > >
> > > I like the approach of only
initializing the table when needed and using
> > > that to control when the
add/remove-thread code needs to update the
> > > table. But I would still want to see
what impact this has on thread
> > > startup cost, both with and without the
table being initialized.
> > >
> > > > The change also includes additional
optimization for some callers of find_JavaThread_from_java_tid()
> > > > as Daniel suggested.
> > >
> > > Not sure it's best to combine these,
but if they are limited to the
> > > changes in management.cpp only then
that may be okay. It helps to be
> > > able to focus on the table related
changes without being distracted by
> > > other optimizations.
> > >
> > > > 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.
> > >
> > > The revised version seems better in
that regard. But I still have a
> > > concern, see below.
> > >
> > > > 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.
> > >
> > > Yes but why? Why can't this table be
grown on demand by the thread that
> > > is doing the addition? For other tables
we may have to delegate to the
> > > service thread because the current
thread cannot perform the action, or
> > > it doesn't want to perform it at the
time the need for the resize is
> > > detected (e.g. its detected at a
safepoint and you want the resize to
> > > happen later outside the safepoint).
It's not apparent to me that such
> > > restrictions apply here.
> > >
> > > > 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.
> > >
> > > Ok.
> > >
> > > > Webrev:
http://cr.openjdk.java.net/~dtitov/8185005/webrev.03
> > >
> > > Some specific code comments:
> > >
> > > src/hotspot/share/runtime/mutexLocker.cpp
> > >
> > > + def(ThreadTableCreate_lock ,
PaddedMutex , special,
> > > false, Monitor::_safepoint_check_never);
> > >
> > > I think this needs to be a
_safepoint_check_always lock. The table will
> > > be created by regular JavaThreads and
they should (nearly) always be
> > > checking for safepoints if they are
going to block acquiring the lock.
> > > And it isn't at all obvious that the
thread doing the creation can't go
> > > to a safepoint whilst this lock is held.
> > >
> > > ---
> > >
> > > src/hotspot/share/runtime/threadSMR.cpp
> > >
> > > Nit:
> > >
> > > 618 JavaThread* thread =
thread_at(i);
> > >
> > > you could reuse the new java_thread
local you introduced at line 613 and
> > > just rename that "new" variable to
"thread" so you don't have to change
> > > all other uses.
> > >
> > > 628 } else if (java_thread != NULL &&
...
> > >
> > > You don't need to check != NULL here as
you only get here when
> > > java_thread is not NULL.
> > >
> > > 755 jlong tid =
SharedRuntime::get_java_tid(thread);
> > > 926 jlong tid =
SharedRuntime::get_java_tid(thread);
> > >
> > > I think it cleaner/better to just use
> > >
> > > jlong tid =
java_lang_Thread::thread_id(thread->threadObj());
> > >
> > > as we know thread is not NULL, it is a
JavaThread and it has to have a
> > > non-null threadObj.
> > >
> > > ---
> > >
> > > src/hotspot/share/services/management.cpp
> > >
> > > 1323 if
(THREAD->is_Java_thread()) {
> > > 1324 JavaThread* current_thread =
(JavaThread*)THREAD;
> > >
> > > These calls can only be made on a
JavaThread so this be simplified to
> > > remove the is_Java_thread() call.
Similarly in other places.
> > >
> > > ---
> > >
> > > src/hotspot/share/services/threadTable.cpp
> > >
> > > 55 class ThreadTableEntry : public
CHeapObj<mtInternal> {
> > > 56 private:
> > > 57 jlong _tid;
> > >
> > > I believe hotspot style is to not
indent the access modifiers in C++
> > > class declarations, so the above would
just be:
> > >
> > > 55 class ThreadTableEntry : public
CHeapObj<mtInternal> {
> > > 56 private:
> > > 57 jlong _tid;
> > >
> > > etc.
> > >
> > > 60 ThreadTableEntry(jlong tid,
JavaThread* java_thread) :
> > > 61
_tid(tid),_java_thread(java_thread) {}
> > >
> > > line 61 should be indented as it
continues line 60.
> > >
> > > 67 class ThreadTableConfig : public
AllStatic {
> > > ...
> > > 71 static uintx get_hash(Value
const& value, bool* is_dead) {
> > >
> > > The is_dead parameter still bothers me
here. I can't make enough sense
> > > out of the template code in
ConcurrentHashtable to see why we have to
> > > have it, but I'm concerned that its
very existence means we perhaps
> > > should not be trying to extend CHT in
this context. ??
> > >
> > > 115 size_t start_size_log =
size_log > DefaultThreadTableSizeLog
> > > 116 ? size_log :
DefaultThreadTableSizeLog;
> > >
> > > line 116 should be indented, though in
this case I think a better layout
> > > would be:
> > >
> > > 115 size_t start_size_log =
> > > 116 size_log >
DefaultThreadTableSizeLog ? size_log :
> > > DefaultThreadTableSizeLog;
> > >
> > > 131 double
ThreadTable::get_load_factor() {
> > > 132 return
(double)_items_count/_current_size;
> > > 133 }
> > >
> > > Not sure that is doing what you
want/expect. It will perform integer
> > > division and then cast that whole
integer to a double. If you want
> > > double arithmetic you need:
> > >
> > > return
((double)_items_count)/_current_size;
> > >
> > > 180 jlong _tid;
> > > 181 uintx _hash;
> > >
> > > Nit: no need for all those spaces
before the variable name.
> > >
> > > 183 ThreadTableLookup(jlong tid)
> > > 184 : _tid(tid),
_hash(primitive_hash(tid)) {}
> > >
> > > line 184 should be indented.
> > >
> > > 201 ThreadGet():_return(NULL) {}
> > >
> > > Nit: need space after :
> > >
> > > 211 assert(_is_initialized, "Thread
table is not initialized");
> > > 212 _has_work = false;
> > >
> > > line 211 is indented one space too far.
> > >
> > > 229 ThreadTableEntry* entry = new
ThreadTableEntry(tid,java_thread);
> > >
> > > Nit: need space after ,
> > >
> > > 252 return
_local_table->remove(thread,lookup);
> > >
> > > Nit: need space after ,
> > >
> > > Thanks,
> > > David
> > > ------
> > >
> > > > Bug:
https://bugs.openjdk.java.net/browse/JDK-8185005
> > > >
> > > > Thanks!
> > > > --Daniil
> > > >
> > > >
> > > > On 7/8/19, 3:24 PM, "Daniel D.
Daugherty" mailto:daniel.daughe...@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:
mailto:serguei.spit...@oracle.com
> > > > > Organization: Oracle Corporation
> > > > > Date: Friday, June 28, 2019 at
7:56 PM
> > > > > To: Daniil Titov
mailto:daniil.x.ti...@oracle.com, OpenJDK Serviceability
mailto:serviceability-dev@openjdk.java.net,
mailto:hotspot-runtime-...@openjdk.java.net
mailto:hotspot-runtime-...@openjdk.java.net,
mailto:jmx-...@openjdk.java.net mailto: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" mailto: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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
>
>
>
>
>
>
>
>
