|
Hi David,
On 9/17/19 03:46, David Holmes wrote:
Hi
Serguei,
On 17/09/2019 7:10 pm, serguei.spit...@oracle.com wrote:
Hi Daniil,
On 9/16/19 21:36, Daniil Titov wrote:
Hi David,
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.
I'm not sure I follow. With the current code if two threads are
racing to initialize the ThreadTable with ThreadsLists that
contain a different set of threads then there are two
possibilities with regards to the interleaving. Assume T1
initializes the table with its set of threads and so finds the tid
it is looking for in the table. Meanwhile T2 is racing with the
initialization logic:
- If T2 sees _is_initialized then lazy_initialization does nothing
for T2, and the additional threads in its ThreadsList (say T3 and
T4) are not added to the table. But the specific thread associated
with the tid (say T3) will be found by linear search of the
ThreadsList and then added. If any other threads come searching
for T4 they too will not find it in the ThreadTable but instead
perform the linear search of their ThreadsList (and add it).
- if T2 doesn't see _is_initialized at first it will try to
acquire the lock, and eventually see _is_initialized is true, at
which point it will try to add all of its thread's to the table
(so T3 and T4 will be added). When lazy_initialize returns, T3
will be found in the table and returned. If any other threads come
searching for T4 they will also find it in the table.
My main concerns are simplicity and reliability.
I do no care much about extra overhead at the ThreadTable
initialization.
A probability of the ThreadTable::lazy_initialize() being called
concurrently is low.
Also, it might happen only once for the whole VM process execution.
I was wrong by thinking that adding new threads to the ThreadTable
after its initialization
will result in thread linear search as well. So my conclusion was
that we should not care
if it happens once more at lazy initialization point.
But now I see that after ThreadTable::is_initialized()
returns true the ThreadsSMRSupport::add_thread()
makes a call to the ThreadTable::add_thread().
So, no more linear search will happen.
However, it seems to me, a possible concurrent lazy initialization
in the webrev.06 introduces
its own extra overhead - competing threads (suppose, we have just
two of them) will do
the same amount of work concurrently:
- all indirect memory readings, lookup/comparisons and other
checks will be performed twice
- new ThreadTableEntry can be allocated twice for some threads in
the list
(depending on how ConcurrentHashTable is implemented, there can
be a potential a memory leak)
So, I doubt we win much in performance here but can loose in
reliability.
I'd suggest to simplify the lazy initialization code and make it
more reliable this way:
if (!_is_initialized) {
MutexLocker ml(ThreadTableCreate_lock);
if (!_is_initialized) {
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 && !thread->is_exiting()) {
jlong java_tid = java_lang_Thread::thread_id(tobj);
add_thread(java_tid, thread);
}
}
}
With
your suggested code change this second case is not possible so for
any racing initialization the lookup of any threads not in the
original ThreadsList will always result in using the linear search
before adding to the table.
Yes, but I did not care about this.
The overhead is expected to be lower than the lazy initialization
cost,
especially because the probability of concurrent initialization is
low.
Both
seem correct to me. Which one is more efficient will totally
depend on the number of differences between the ThreadsLists and
whether the code ever tries to look up those additional threads.
If we assume racing initialization is likely to be rare anyway
(because generally one thread is in charge of doing the
monitoring) then the choice seems somewhat arbitrary.
I agree, but have a little preference in favor of simplicity.
It was a good discussion though. :)
Thanks,
Serguei
Cheers,
David
-----
Thanks,
Serguei
The
assumption is that it's quite uncommon and even if this is the
case the linear scan happens
only once per such thread.
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 }
Thanks,
Daniil
On 9/16/19, 7:27 PM, "David Holmes"
<david.hol...@oracle.com> wrote:
Hi Daniil,
Thanks again for your perseverance on this one.
I think there is a problem with initialization of the
thread table.
Suppose thread T1 has called
ThreadsList::find_JavaThread_from_java_tid
and has commenced execution of
ThreadTable::lazy_initialize, but not yet
marked _is_initialized as true. Now two new threads (T2
and T3) are
created and start running - they aren't added to the
ThreadTable yet
because it isn't initialized. Now T0 also calls
ThreadsList::find_JavaThread_from_java_tid using an
updated ThreadsList
that contains T2 and T3. It also calls
ThreadTable::lazy_initialize. If
_is_initialized is still false T0 will attempt
initialization but once
it gets the lock it will see the table has now been
initialized by T1.
It will then proceed to update the table with its own
ThreadList content
- adding T2 and T3. That is all fine. But now suppose T0
initially sees
_is_initialized as true, it will do nothing in
lazy_initialize and
simply return to find_JavaThread_from_java_tid. But now
T2 and T3 are
missing from the ThreadTable and nothing will cause them
to be added.
More generally any ThreadsList that is created after the
ThreadsList
that will be used for initialization, may contain threads
that will not
be added to the table.
Thanks,
David
On 17/09/2019 4:18 am, 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"
<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" <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" <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:
<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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
>
>
>
>
|
