Hi David, Robbin, Daniel, and Serguei,
Please review a new version of the fix.
As David suggested I created a separated Jira issue [1] to cover additional
optimization for
some callers of find_JavaThread_from_java_tid() and this version of the fix no
longer includes
changes in management.cpp ( and the test related with these changes).
Regarding the impact the previous version of the fix had on the thread startup
time at heavy load (e.g.
when 5000 threads are created and destroyed every second) I tried a different
approach that makes
calls to ThreadTable::add_thread and ThreadTable::remove_thread asynchronous
and offloads the
work for actual modifications of the thread table to a periodic task that runs
every 5 seconds. With the
same stress test scenario (the test does some warm-up and then measures the
time it takes to create
and start 100,000 threads; every thread just sleeps for 100 ms) the impact on
the thread startup time
was reduced to 1.2% ( from 2.7%).
The cause of this impact in this stress test scenario is that as soon as the
thread table is initialized,
an additional work to insert and delete entries in the thread table should be
performed, even if
com.sun.management.ThreadMXBean methods are no longer called. For example, In
the stress test
mentioned above, every second about 5000 entries had to be inserted in the
table and then deleted.
That doesn't look right and the new version of the fix uses the different
approach: the thread is added to
the thread table only when this thread is requested by
com.sun.management.ThreadMXBean bean. Every
time when find_JavaThread_from_java_tid() is called for a new tid, the thread
is found by the iterating over
the thread list and added to the thread table. All consequent calls to
find_JavaThread_from_java_tid() for
the same tid returns the thread from the thread table.
Running stress test for the cases when the thread table is enabled and not
showed no difference in the
average thread startup times.
[1] : https://bugs.openjdk.java.net/browse/JDK-8229391
Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.05/
Thanks,
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
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
>
>
>
