8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Aug 14 18:25:07 UTC 2019
So based on the last three message on this thread:
webrev.05 is withdrawn for the moment
webrev.04 is the current webrev, but needs to have some startup time
issues resolved before moving forward.
I'm going to hold off on re-reviewing anything at the moment until
the dust settles...
Dan
On 8/12/19 7:34 PM, David Holmes wrote:
> Hi Daniil,
>
> On 13/08/2019 9:24 am, Daniil Titov wrote:
>> Hi Robbin,
>>
>> Thank you very much for reviewing this version of the fix! Based on
>> your findings
>> it seems as it makes sense to make a step back and continue with the
>> approach we took before in the previous version of the webrev
>> (webrev.04),
>> and get more information about the impact on the startup time it has.
>> I will
>> consult with Claus regarding this and then share the findings.
>
> That seems a good approach to me. It wasn't at all clear to me that
> the latest proposed approach would actually solve the original problem
> in a satisfactory way - it would depend on how constant the set of
> threads being queried was.
>
> There is no perfect solution here as any fix to the reported problem
> incurs overhead elsewhere. Even evaluating the merits of the different
> trade-offs is hard to do - we could end up with a compromise solution
> that fails to satisfy anyone.
>
> David
> -----
>
>> Thanks again,
>> --Daniil
>>
>>
>>
>>
>>
>>
>> On 8/12/19, 5:22 AM, "Robbin Ehn" <robbin.ehn at oracle.com> wrote:
>>
>> Hi Daniil,
>> I took a new deeper dive into this.
>> This line seems to have some issues:
>> if (ThreadTable::is_initialized() &&
>> thread->in_thread_table() &&
>> !thread->is_attaching_via_jni()) {
>> If you create new threads which attaches and then dies, the
>> table will just keep
>> growing. So you must remove them also ?
>> Secondly you should not use volatile semantics for
>> _in_thread_table.
>> The load in the if-statement can be reordered with _is_initialized.
>> Which could lead to a leak, rogue pointer in the table.
>> So both "static volatile bool _is_initialized;" and
>> "volatile bool
>> _in_thread_table; "
>> should be stored with store_release and loaded with load_acquire.
>> Unfortunately it looks like there still would be races if
>> ThreadTable::add_thread e.g. context switch at:
>> if (_local_table->insert(thread, lookup, entry)) {
>> // HERE
>> java_thread->set_in_thread_table(true);
>> *Remove side can pass the if-statement without removing.
>> Since this thread also maybe exiting at any moment, e.g.
>> context switch:
>> if (tobj != NULL && !thread->is_exiting() &&
>> java_tid == java_lang_Thread::thread_id(tobj)) {
>> // HERE
>> ThreadTable::add_thread(java_tid, thread);
>> *Add side can add a thread that is exiting.
>> Mixing in a third thread looking up a random tid and
>> getting a JavaThread*, it
>> must validate it against it's ThreadsList. Making the hashtable
>> useless.
>> So I think the only one adding and removing should be the
>> thread itself.
>> 1:Add to ThreadsList
>> 2:Add to ThreadTable
>> 3:Remove from ThreadTable
>> 4:Remove ThreadsList
>> Between 1-2 and 3-4 the thread would be looked-up via
>> linear scan.
>> I don't see an easy way around the start-up issue with this.
>> Maybe have the cache in Java.
>> Pass in the thread obj into a
>> java_sun_management_ThreadImpl_getThreadTotalCpuTime3 instead,
>> thus skipping any look-ups in native.
>> Thanks, Robbin
>> On 8/12/19 5:49 AM, Daniil Titov wrote:
>> > Hi David, Robbin, Daniel, and Serguei,
>> >
>> > Please review a new version of the fix.
>> >
>> > As David suggested I created a separated Jira issue [1] to
>> cover additional optimization for
>> > some callers of find_JavaThread_from_java_tid() and this
>> version of the fix no longer includes
>> > changes in management.cpp ( and the test related with these
>> changes).
>> >
>> > Regarding the impact the previous version of the fix had on
>> the thread startup time at heavy load (e.g.
>> > when 5000 threads are created and destroyed every second) I
>> tried a different approach that makes
>> > calls to ThreadTable::add_thread and
>> ThreadTable::remove_thread asynchronous and offloads the
>> > work for actual modifications of the thread table to a
>> periodic task that runs every 5 seconds. With the
>> > same stress test scenario (the test does some warm-up and
>> then measures the time it takes to create
>> > and start 100,000 threads; every thread just sleeps for 100
>> ms) the impact on the thread startup time
>> > was reduced to 1.2% ( from 2.7%).
>> >
>> > The cause of this impact in this stress test scenario is that
>> as soon as the thread table is initialized,
>> > an additional work to insert and delete entries in the thread
>> table should be performed, even if
>> > com.sun.management.ThreadMXBean methods are no longer called.
>> For example, In the stress test
>> > mentioned above, every second about 5000 entries had to be
>> inserted in the table and then deleted.
>> >
>> > That doesn't look right and the new version of the fix uses
>> the different approach: the thread is added to
>> > the thread table only when this thread is requested by
>> com.sun.management.ThreadMXBean bean. Every
>> > time when find_JavaThread_from_java_tid() is called for a new
>> tid, the thread is found by the iterating over
>> > the thread list and added to the thread table. All consequent
>> calls to find_JavaThread_from_java_tid() for
>> > the same tid returns the thread from the thread table.
>> >
>> > Running stress test for the cases when the thread table is
>> enabled and not showed no difference in the
>> > average thread startup times.
>> >
>> > [1] : https://bugs.openjdk.java.net/browse/JDK-8229391
>> >
>> > Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
>> > Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.05/
>> >
>> > Thanks,
>> > Daniil
>> >
>> > On 8/4/19, 7:54 PM, "David Holmes" <david.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 serviceability-dev
mailing list