RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Sat Sep 21 00:07:01 UTC 2019
On 9/20/19 4:50 PM, serguei.spitsyn at oracle.com wrote:
> Hi Daniil,
>
> Yes, the Threads_lock is still needed around thread->is_exiting()
> check and add_thread().
>
>> And if we try to use 2 locks, ThreadTableCreate_lock as in your snippet
>> and then the nested Threads_lock around thread->is_exiting() and
>> add_thread(java_tid, thread) lines then it will not work since the rank
>> of Threads_lock is higher than the rank of ThreadTableCreate_lock.
>
> The ThreadTableCreate_lock is only used in the lazy_initialize()
> function.
> It looks safe to adjust the ThreadTableCreate_lock to fix the above
> problem.
Sorry, I wanted to say: "to adjust the ThreadTableCreate_lock rank".
Thanks,
Serguei
> Then the approach below will work as needed.
>
> void ThreadTable::lazy_initialize(const ThreadsList *threads) {
> if (_is_initialized) {
> return;
> }
> MutexLocker ml(ThreadTableCreate_lock);
> if (_is_initialized) {
> // There is no obvious benefits in allowing the thread table
> // being concurrently populated during the initalization.
> return;
> }
> 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) {
> jlong java_tid = java_lang_Thread::thread_id(tobj);
> MutexLocker ml(Threads_lock);
> if (!thread->is_exiting()) {
> // Must be inside the lock to ensure that we don't add the
> thread to the table
> // that has just passed the removal point in
> ThreadsSMRSupport::remove_thread()
> add_thread(java_tid, thread);
> }
> }
> }
> }
>
>
> If you rename ThreadTable to ThreadIdTable then the
> ThreadTableCreate_lock
> has to be renamed to ThreadIdTableCreate_lock.
>
> Thanks,
> Serguei
>
> On 9/20/19 8:42 AM, Daniil Titov wrote:
>> Hi Serguei,
>>
>>> void ThreadTable::lazy_initialize(const ThreadsList *threads) {
>>> if (_is_initialized) {
>>> return;
>> > }
>>> MutexLocker ml(ThreadTableCreate_lock);
>>
>> If I understood you correctly in the code snippet you sent you meant
>> to use
>> Threads_lock, not ThreadTableCreate_lock, right?
>>
>> The original idea was to do a minimal amount of work while holding
>> the lock and
>> hold the lock for as short period of time as possible to not block
>> other threads when it is not necessary.
>>
>> With the suggested approach no new threads could be started until the
>> thread table is created and
>> populated with all threads running inside a Java application and in
>> case of large app there could be
>> thousands of them.
>>
>> And if we try to use 2 locks, ThreadTableCreate_lock as in your
>> snippet and then the nested Threads_lock around
>> thread->is_exiting() and add_thread(java_tid, thread) lines then it
>> will not work since the rank of Threads_lock
>> is higher than the rank of ThreadTableCreate_lock.
>>
>> So choosing between blocking new threads from starting and
>> potentially allowing
>> some other monitoring thread to do a one-time linear scan I think it
>> makes sense to choose the latter.
>>
>> Thanks!
>>
>> Best regards,
>> Daniil
>>
>>
>>
>> From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> Date: Thursday, September 19, 2019 at 10:30 PM
>> To: Daniil Titov <daniil.x.titov at oracle.com>, Robbin Ehn
>> <robbin.ehn at oracle.com>, David Holmes <david.holmes at oracle.com>,
>> <daniel.daugherty 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>, Claes Redestad <claes.redestad at oracle.com>
>> Subject: Re: RFR: 8185005: Improve performance of
>> ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
>>
>> Hi Daniil,
>>
>> I think, it is better to grab the thread_lock just once at lazy
>> initialization.
>> It would look simpler, something, like this would work:
>> void ThreadTable::lazy_initialize(const ThreadsList *threads) {
>> if (_is_initialized) {
>> return;
>> }
>> MutexLocker ml(ThreadTableCreate_lock);
>> if (_is_initialized) {
>> // There is no obvious benefits in allowing the thread table
>> // being concurrently populated during the initalization.
>> return;
>> }
>> 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) {
>> jlong java_tid = java_lang_Thread::thread_id(tobj);
>> if (!thread->is_exiting()) {
>> // Must be inside the lock to ensure that we don't add the
>> thread to the table
>> // that has just passed the removal point in
>> ThreadsSMRSupport::remove_thread()
>> add_thread(java_tid, thread);
>> }
>> }
>> }
>> }
>>
>> Otherwise, concurrent executions of the find_JavaThread_from_java_tid()
>> will sometimes do a linear search of threads that are not included
>> yet to
>> the ThreadTable from the ThreadsList (which is used for lazy
>> initialization).
>> Instead, it is better to wait for the lazy_initialization() to complete.
>> Thanks,
>> Serguei
>>
>>
>> On 9/19/19 17:30, Daniil Titov wrote:
>> Hi David and Serguei,
>>
>> Please review new version of the fix that includes the changes
>> Serguei suggested:
>> 1. If racing threads initialize the thread table only one of these
>> threads will populate the table with the threads from the thread list
>> 2. The code that adds the thread to the tread table is put inside
>> Threads_lock to ensure that we cannot accidentally add the thread
>> that has just passed the removal point in
>> ThreadsSMRSupport::remove_thread()
>>
>> The changes are in ThreadTable::lazy_initialize() method only.
>>
>> Testing: Mach5 tier1, tier2, tier3, tier4, and tier5 tests
>> successfully passed.
>>
>> Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.07/
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8185005
>>
>> Thank you!
>> --Daniil
>>
>> On 9/18/19, 1:01 AM, mailto:serguei.spitsyn at oracle.com
>> mailto:serguei.spitsyn at oracle.com wrote:
>>
>> Hi Daniil,
>> On 9/17/19 17:13, Daniil Titov wrote:
>> > Hi Serguei,
>> >
>> > Please find below my answers to the concerns you mentioned in
>> the previous email.
>> >
>> > 1.
>> > > I have a concern about the checks for thread->is_exiting().
>> > > - the lines 632-633 are useless as they do not really
>> protect from returning an exiting thread
>> >> It is interesting what might happen if an exiting thread is
>> returned by the
>> >> ThreadsList::find_JavaThread_from_java_tid ().
>> >> Does it make sense to develop a test that would cover these
>> cases?
>> > I agree, it doesn't really provide any protection so it makes
>> sense just remove it.
>> Now, I'm not that confident about it. :)
>> > The current implementation
>> > find_JavaThread_from_java_tid() doesn't provide such
>> protection as well, since the thread could start exiting
>> > immediately after method find_JavaThread_from_java_tid()
>> returns, so the assumption is that the callers of
>> > find_JavaThread_from_java_tid() are expecting to deal with
>> such threads and looking on some of them shows that
>> > they usually try to retrieve threadObj or a thread statistic
>> object and if it is NULL that just do nothing.
>> If I understand it correctly, the jt->threadObj() can
>> remain non-NULL
>> for some time while jt->is_exiting() == true.
>> It is not clear how reliable is to use it.
>> But this is a pre-existing issue. It is not you who introduced
>> it. :)
>> So, we can skip it for now.
>> But for the record, we may have a source of intermittent issues.
>> > I'm not sure we could cover this specific case with the
>> test. The window between find_JavaThread_from_java_tid() returns and
>> the caller
>> > continues the execution is too small. The window between the
>> thread started exiting and removed itself from the thread table is
>> very small as well.
>> Understand.
>> > 2.
>> >> - the lines 105-108 can result in adding exiting threads
>> into the ThreadTable
>> > I agree, it was missed, we need to wrap this code inside
>> Thread_lock in the similar way as it is done
>> find_JavaThread_from_java_tid()
>> Okay, thanks!
>> > 3.
>> >> I would suggest to rewrite this fragment in a safe way:
>> >> 95 {
>> >> 96 MutexLocker ml(ThreadTableCreate_lock);
>> >> 97 if (!_is_initialized) {
>> >> 98 create_table(threads->length());
>> >> 99 _is_initialized = true;
>> >> 100 }
>> >> 101 }
>> >> as:
>> >> {
>> >> MutexLocker ml(ThreadTableCreate_lock);
>> >> if (_is_initialized) {
>> >> return;
>> > > }
>> > > create_table(threads->length());
>> > > _is_initialized = true;
>> > > }
>> >
>> > It was an intension to not block while populating the table
>> with the threads from the current thread list.
>> > There is no needs to have other threads that call
>> find_JavaThread_from_java_tid() be blocked and waiting for
>> > it to complete since the requested thread could be not
>> present in the thread list that triggers the thread table
>> > initialization. Plus in case of racing initialization it
>> allows threads from not original thread lists be added to the table
>> > and thus avoid the linear scan when these thread are looked up
>> for the first time.
>> I've replied to David in another email.
>> Let's talk once more about it tomorrow.
>> > 4.
>> >>> 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.
>> > David Holmes replied to this in a separate email providing a
>> very detailed
>> > explanation of the possible cases and how the proposed
>> implementation satisfies them.
>> Yes. Please, see above.
>> Thanks,
>> Serguei
>> > Best regards,
>> > Daniil
>> >
>> > From: mailto:serguei.spitsyn at oracle.com
>> mailto:serguei.spitsyn at oracle.com
>> > Date: Tuesday, September 17, 2019 at 1:53 AM
>> > To: Daniil Titov mailto:daniil.x.titov at oracle.com, Robbin Ehn
>> mailto:robbin.ehn at oracle.com, David Holmes
>> mailto:david.holmes at oracle.com, mailto:daniel.daugherty at oracle.com,
>> OpenJDK Serviceability mailto:serviceability-dev at openjdk.java.net,
>> mailto:hotspot-runtime-dev at openjdk.java.net
>> mailto:hotspot-runtime-dev at openjdk.java.net,
>> mailto:jmx-dev at openjdk.java.net mailto:jmx-dev at openjdk.java.net,
>> Claes Redestad mailto:claes.redestad at oracle.com
>> > Subject: Re: RFR: 8185005: Improve performance of
>> ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
>> >
>> > Hi Daniil,
>> >
>> > Thank you for you patience in working on this issue!
>> > Also, I like that the current thread related optimizations in
>> management.cpp were factored out.
>> > It was a good idea to separate them.
>> >
>> > I have a concern about the checks for thread->is_exiting().
>> > The threads are added to and removed from the ThreadTable
>> under protection of Threads_lock.
>> > However, the thread->is_exiting() checks are not protected,
>> and so, they are racy.
>> >
>> > There is a couple of such checks to mention:
>> > 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 }
>> > ...
>> > 93 void ThreadTable::lazy_initialize(const ThreadsList
>> *threads) {
>> > 94 if (!_is_initialized) {
>> > 95 {
>> > 96 MutexLocker ml(ThreadTableCreate_lock);
>> > 97 if (!_is_initialized) {
>> > 98 create_table(threads->length());
>> > 99 _is_initialized = true;
>> > 100 }
>> > 101 }
>> > 102 for (uint i = 0; i < threads->length(); i++) {
>> > 103 JavaThread* thread = threads->thread_at(i);
>> > 104 oop tobj = thread->threadObj();
>> > 105 if (tobj != NULL && !thread->is_exiting()) {
>> > 106 jlong java_tid = java_lang_Thread::thread_id(tobj);
>> > 107 add_thread(java_tid, thread);
>> > 108 }
>> > 109 }
>> > 110 }
>> > 111 }
>> >
>> > A thread may start exiting right after the checks at the lines
>> 626 and 105.
>> > So that:
>> > - the lines 632-633 are useless as they do not really
>> protect from returning an exiting thread
>> > - the lines 105-108 can result in adding exiting threads
>> into the ThreadTable
>> >
>> > Please, note, the lines 626-629 are safe in terms of addition
>> to the ThreadTable as they
>> > are protected with the Threads_lock. But the returned thread
>> still can exit after that.
>> > It is interesting what might happen if an exiting thread is
>> returned by the
>> > ThreadsList::find_JavaThread_from_java_tid ().
>> >
>> > Does it make sense to develop a test that would cover these
>> cases?
>> >
>> > Thanks,
>> > Serguei
>> >
>> >
>> > On 9/16/19 11:18, 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"
>> mailto: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"
>> mailto: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" mailto: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:
>> mailto:serguei.spitsyn at oracle.com
>> > > > > > Organization: Oracle Corporation
>> > > > > > Date: Friday, June 28, 2019 at
>> 7:56 PM
>> > > > > > To: Daniil Titov
>> mailto:daniil.x.titov at oracle.com, OpenJDK Serviceability
>> mailto:serviceability-dev at openjdk.java.net,
>> mailto:hotspot-runtime-dev at openjdk.java.net
>> mailto:hotspot-runtime-dev at openjdk.java.net,
>> mailto:jmx-dev at openjdk.java.net mailto: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" mailto: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