8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Sep 18 07:34:55 UTC 2019
Hi David,
On 9/18/19 00:25, David Holmes wrote:
> Hi Serguei,
>
> In the interests of full disclosure I was the one who told Daniil to
> not hold the lock while populating the table.
I well understand your point to not hold the lock while populating the
table.
>
> I'll leave you two to work out which way you want to go there.
In fact, I have no strong opinion here, just my gut feelings. :)
And I'm open for any solution.
Just wanted to share my thoughts with you, guys, to have all reasoning
on the table.
Let me briefly talk to Daniil tomorrow.
Thanks,
Serguei
> Thanks,
> David
>
> On 18/09/2019 5:13 pm, serguei.spitsyn at oracle.com wrote:
>> Hi David,
>>
>>
>> On 9/17/19 03:46, David Holmes wrote:
>>> Hi Serguei,
>>>
>>> On 17/09/2019 7:10 pm, serguei.spitsyn at 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.holmes at 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.holmes at 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.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 hotspot-runtime-dev
mailing list