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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >