RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
David Holmes
david.holmes at oracle.com
Mon Aug 5 02:54:07 UTC 2019
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.holmes at 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.daugherty at 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 at oracle.com>
> > > Organization: Oracle Corporation
> > > Date: Friday, June 28, 2019 at 7:56 PM
> > > To: Daniil Titov <daniil.x.titov at oracle.com>, OpenJDK Serviceability <serviceability-dev at openjdk.java.net>, "hotspot-runtime-dev at openjdk.java.net" <hotspot-runtime-dev at openjdk.java.net>, "jmx-dev at openjdk.java.net" <jmx-dev at 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 at 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
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
>
>
>
More information about the serviceability-dev
mailing list