Thanks again,
--Daniil
On 8/12/19, 5:22 AM, "Robbin Ehn" <robbin....@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.hol...@oracle.com> wrote:
>
> Hi Daniil,
>
> On 3/08/2019 8:16 am, Daniil Titov wrote:
> > Hi David,
> >
> > Thank you for your detailed review. Please review a new version
of the fix that includes
> > the changes you suggested:
> > - ThreadTableCreate_lock scope is reduced to cover the creation
of the table only;
> > - ThreadTableCreate_lock is made _safepoint_check_always;
>
> Okay.
>
> > - ServiceThread is no longer responsible for the resizing of the
thread table, instead,
> > the thread table is changed to grow on demand by the thread
that is doing the addition;
>
> Okay - I'm happy to get the serviceThread out of the picture here.
>
> > - fixed nits and formatting issues.
>
> Okay.
>
> >>> The change also includes additional optimization for some
callers of find_JavaThread_from_java_tid()
> >>> as Daniel suggested.
> >> Not sure it's best to combine these, but if they are limited to
the
> >> changes in management.cpp only then that may be okay.
> >
> > The additional optimization for some callers of
find_JavaThread_from_java_tid() is
> > limited to management.cpp (plus a new test) so I left them in the
webrev but
> > I also could move it in the separate issue if required.
>
> I'd prefer this part of be separated out, but won't insist. Let's
see if
> Dan or Serguei have a strong opinion.
>
> > > src/hotspot/share/runtime/threadSMR.cpp
> > >755 jlong tid = SharedRuntime::get_java_tid(thread);
> > > 926 jlong tid = SharedRuntime::get_java_tid(thread);
> > > I think it cleaner/better to just use
> > > jlong tid = java_lang_Thread::thread_id(thread->threadObj());
> > > as we know thread is not NULL, it is a JavaThread and it has
to have a
> > > non-null threadObj.
> >
> > I had to leave this code unchanged since it turned out the
threadObj is null
> > when VM is destroyed:
> >
> > V [libjvm.so+0xe165d7] oopDesc::long_field(int) const+0x67
> > V [libjvm.so+0x16e06c6]
ThreadsSMRSupport::add_thread(JavaThread*)+0x116
> > V [libjvm.so+0x16d1302] Threads::add(JavaThread*, bool)+0x82
> > V [libjvm.so+0xef8369] attach_current_thread.part.197+0xc9
> > V [libjvm.so+0xec136c] jni_DestroyJavaVM+0x6c
> > C [libjli.so+0x4333] JavaMain+0x2c3
> > C [libjli.so+0x8159] ThreadJavaMain+0x9
>
> This is actually nothing to do with the VM being destroyed, but is
an
> issue with JNI_AttachCurrentThread and its interaction with the
> ThreadSMR iterators. The attach process is:
> - create JavaThread
> - mark as "is attaching via jni"
> - add to ThreadsList
> - create java.lang.Thread object (you can only execute Java code
after
> you are attached)
> - mark as "attach completed"
>
> So while a thread "is attaching" it will be seen by the ThreadSMR
thread
> iterator but will have a NULL java.lang.Thread object.
>
> We special-case attaching threads in a number of places in the VM
and I
> think we should be explicitly doing something here to filter out
> attaching threads, rather than just being tolerant of a NULL
j.l.Thread
> object. Specifically in ThreadsSMRSupport::add_thread:
>
> if (ThreadTable::is_initialized() &&
!thread->is_attaching_via_jni()) {
> jlong tid = java_lang_Thread::thread_id(thread->threadObj());
> ThreadTable::add_thread(tid, thread);
> }
>
> Note that in ThreadsSMRSupport::remove_thread we can use the same
guard,
> which covers the case the JNI attach encountered an error trying to
> create the j.l.Thread object.
>
> >> src/hotspot/share/services/threadTable.cpp
> >> 71 static uintx get_hash(Value const& value, bool* is_dead) {
> >
> >> The is_dead parameter still bothers me here. I can't make enough
sense
> >> out of the template code in ConcurrentHashtable to see why we
have to
> >> have it, but I'm concerned that its very existence means we
perhaps
> >> should not be trying to extend CHT in this context. ??
> >
> > My understanding is that is_dead parameter provides a mechanism
for
> > ConcurrentHashtable to remove stale entries that were not
explicitly
> > removed by calling ConcurrentHashTable::remove() method.
> > I think that just because in our case we don't use this mechanism
doesn't
> > mean we should not use ConcurrentHashTable.
>
> Can you confirm that this usage is okay with Robbin Ehn please. He's
> back from vacation this week.
>
> >> I would still want to see what impact this has on thread
> >> startup cost, both with and without the table being initialized.
> >
> > I run a test that initializes the table by calling
ThreadMXBean.get getThreadInfo(),
> > starts some threads as a worm-up, and then creates and starts
100,000 threads
> > (each thread just sleeps for 100 ms). In case when the thread
table is enabled
> > 100,000 threads are created and started for about 15200 ms. If
the thread table
> > is off the test takes about 14800 ms. Based on this information
the enabled
> > thread table makes the thread startup about 2.7% slower.
>
> That doesn't sound very good. I think we may need to Claes involved
to
> help investigate overall performance impact here.
>
> > Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.04/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
>
> No further code comments.
>
> I didn't look at the test in detail.
>
> Thanks,
> David
>
> > Thanks!
> > --Daniil
> >
> >
> > On 7/29/19, 12:53 AM, "David Holmes" <david.hol...@oracle.com>
wrote:
> >
> > Hi Daniil,
> >
> > Overall I think this is a reasonable approach but I would
still like to
> > see some performance and footprint numbers, both to verify
it fixes the
> > problem reported, and that we are not getting penalized
elsewhere.
> >
> > On 25/07/2019 3:21 am, Daniil Titov wrote:
> > > Hi David, Daniel, and Serguei,
> > >
> > > Please review the new version of the fix, that makes the
thread table initialization on demand and
> > > moves it inside
ThreadsList::find_JavaThread_from_java_tid(). At the creation time the thread table
> > > is initialized with the threads from the current thread
list. We don't want to hold Threads_lock
> > > inside find_JavaThread_from_java_tid(), thus new threads
still could be created while the thread
> > > table is being initialized . Such threads will be found by
the linear search and added to the thread table
> > > later, in ThreadsList::find_JavaThread_from_java_tid().
> >
> > The initialization allows the created but unpopulated, or
partially
> > populated, table to be seen by other threads - is that your
intention?
> > It seems it should be okay as the other threads will then
race with the
> > initializing thread to add specific entries, and this is a
concurrent
> > map so that should be functionally correct. But if so then I
think you
> > can also reduce the scope of the ThreadTableCreate_lock so
that it
> > covers creation of the table only, not the initial
population of the table.
> >
> > I like the approach of only initializing the table when
needed and using
> > that to control when the add/remove-thread code needs to
update the
> > table. But I would still want to see what impact this has on
thread
> > startup cost, both with and without the table being
initialized.
> >
> > > The change also includes additional optimization for some
callers of find_JavaThread_from_java_tid()
> > > as Daniel suggested.
> >
> > Not sure it's best to combine these, but if they are limited
to the
> > changes in management.cpp only then that may be okay. It
helps to be
> > able to focus on the table related changes without being
distracted by
> > other optimizations.
> >
> > > That is correct that ResolvedMethodTable was used as a
blueprint for the thread table, however, I tried
> > > to strip it of the all functionality that is not required
in the thread table case.
> >
> > The revised version seems better in that regard. But I still
have a
> > concern, see below.
> >
> > > We need to have the thread table resizable and allow it to
grow as the number of threads increases to avoid
> > > reserving excessive memory a-priori or deteriorating
lookup times. The ServiceThread is responsible for
> > > growing the thread table when required.
> >
> > Yes but why? Why can't this table be grown on demand by the
thread that
> > is doing the addition? For other tables we may have to
delegate to the
> > service thread because the current thread cannot perform the
action, or
> > it doesn't want to perform it at the time the need for the
resize is
> > detected (e.g. its detected at a safepoint and you want the
resize to
> > happen later outside the safepoint). It's not apparent to me
that such
> > restrictions apply here.
> >
> > > There is no ConcurrentHashTable available in Java 8 and
for backporting this fix to Java 8 another implementation
> > > of the hash table, probably originally suggested in the
patch attached to the JBS issue, should be used. It will make
> > > the backporting more complicated, however, adding a new
Implementation of the hash table in Java 14 while it
> > > already has ConcurrentHashTable doesn't seem reasonable
for me.
> >
> > Ok.
> >
> > > Webrev:
http://cr.openjdk.java.net/~dtitov/8185005/webrev.03
> >
> > Some specific code comments:
> >
> > src/hotspot/share/runtime/mutexLocker.cpp
> >
> > + def(ThreadTableCreate_lock , PaddedMutex ,
special,
> > false, Monitor::_safepoint_check_never);
> >
> > I think this needs to be a _safepoint_check_always lock. The
table will
> > be created by regular JavaThreads and they should (nearly)
always be
> > checking for safepoints if they are going to block acquiring
the lock.
> > And it isn't at all obvious that the thread doing the
creation can't go
> > to a safepoint whilst this lock is held.
> >
> > ---
> >
> > src/hotspot/share/runtime/threadSMR.cpp
> >
> > Nit:
> >
> > 618 JavaThread* thread = thread_at(i);
> >
> > you could reuse the new java_thread local you introduced at
line 613 and
> > just rename that "new" variable to "thread" so you don't
have to change
> > all other uses.
> >
> > 628 } else if (java_thread != NULL && ...
> >
> > You don't need to check != NULL here as you only get here
when
> > java_thread is not NULL.
> >
> > 755 jlong tid = SharedRuntime::get_java_tid(thread);
> > 926 jlong tid = SharedRuntime::get_java_tid(thread);
> >
> > I think it cleaner/better to just use
> >
> > jlong tid = java_lang_Thread::thread_id(thread->threadObj());
> >
> > as we know thread is not NULL, it is a JavaThread and it has
to have a
> > non-null threadObj.
> >
> > ---
> >
> > src/hotspot/share/services/management.cpp
> >
> > 1323 if (THREAD->is_Java_thread()) {
> > 1324 JavaThread* current_thread =
(JavaThread*)THREAD;
> >
> > These calls can only be made on a JavaThread so this be
simplified to
> > remove the is_Java_thread() call. Similarly in other places.
> >
> > ---
> >
> > src/hotspot/share/services/threadTable.cpp
> >
> > 55 class ThreadTableEntry : public CHeapObj<mtInternal> {
> > 56 private:
> > 57 jlong _tid;
> >
> > I believe hotspot style is to not indent the access
modifiers in C++
> > class declarations, so the above would just be:
> >
> > 55 class ThreadTableEntry : public CHeapObj<mtInternal> {
> > 56 private:
> > 57 jlong _tid;
> >
> > etc.
> >
> > 60 ThreadTableEntry(jlong tid, JavaThread*
java_thread) :
> > 61 _tid(tid),_java_thread(java_thread) {}
> >
> > line 61 should be indented as it continues line 60.
> >
> > 67 class ThreadTableConfig : public AllStatic {
> > ...
> > 71 static uintx get_hash(Value const& value, bool*
is_dead) {
> >
> > The is_dead parameter still bothers me here. I can't make
enough sense
> > out of the template code in ConcurrentHashtable to see why
we have to
> > have it, but I'm concerned that its very existence means we
perhaps
> > should not be trying to extend CHT in this context. ??
> >
> > 115 size_t start_size_log = size_log >
DefaultThreadTableSizeLog
> > 116 ? size_log : DefaultThreadTableSizeLog;
> >
> > line 116 should be indented, though in this case I think a
better layout
> > would be:
> >
> > 115 size_t start_size_log =
> > 116 size_log > DefaultThreadTableSizeLog ? size_log :
> > DefaultThreadTableSizeLog;
> >
> > 131 double ThreadTable::get_load_factor() {
> > 132 return (double)_items_count/_current_size;
> > 133 }
> >
> > Not sure that is doing what you want/expect. It will perform
integer
> > division and then cast that whole integer to a double. If
you want
> > double arithmetic you need:
> >
> > return ((double)_items_count)/_current_size;
> >
> > 180 jlong _tid;
> > 181 uintx _hash;
> >
> > Nit: no need for all those spaces before the variable name.
> >
> > 183 ThreadTableLookup(jlong tid)
> > 184 : _tid(tid), _hash(primitive_hash(tid)) {}
> >
> > line 184 should be indented.
> >
> > 201 ThreadGet():_return(NULL) {}
> >
> > Nit: need space after :
> >
> > 211 assert(_is_initialized, "Thread table is not
initialized");
> > 212 _has_work = false;
> >
> > line 211 is indented one space too far.
> >
> > 229 ThreadTableEntry* entry = new
ThreadTableEntry(tid,java_thread);
> >
> > Nit: need space after ,
> >
> > 252 return _local_table->remove(thread,lookup);
> >
> > Nit: need space after ,
> >
> > Thanks,
> > David
> > ------
> >
> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8185005
> > >
> > > Thanks!
> > > --Daniil
> > >
> > >
> > > On 7/8/19, 3:24 PM, "Daniel D. Daugherty"
<daniel.daughe...@oracle.com> wrote:
> > >
> > > On 6/29/19 12:06 PM, Daniil Titov wrote:
> > > > Hi Serguei and David,
> > > >
> > > > Serguei is right,
ThreadTable::find_thread(java_tid) cannot return a JavaThread with an unmatched java_tid.
> > > >
> > > > Please find a new version of the fix that includes
the changes Serguei suggested.
> > > >
> > > > Regarding the concern about the maintaining the
thread table when it may never even be queried, one of
> > > > the options could be to add ThreadTable ::isEnabled flag, set
it to "false" by default, and wrap the calls to the thread table
> > > > in ThreadsSMRSupport add_thread() and
remove_thread() methods to check this flag.
> > > >
> > > > When ThreadsList::find_JavaThread_from_java_tid()
is called for the first time it could check if ThreadTable ::isEnabled
> > > > Is on and if not then set it on and populate the
thread table with all existing threads from the thread list.
> > >
> > > I have the same concerns as David H. about this new
ThreadTable.
> > > ThreadsList::find_JavaThread_from_java_tid() is only
called from code
> > > in src/hotspot/share/services/management.cpp so I
think that table
> > > needs to enabled and populated only if it is going to
be used.
> > >
> > > I've taken a look at the webrev below and I see that
David has
> > > followed up with additional comments. Before I do a
crawl through
> > > code review for this, I would like to see the
ThreadTable stuff
> > > made optional and David's other comments addressed.
> > >
> > > Another possible optimization is for callers of
> > > find_JavaThread_from_java_tid() to save the calling
thread's
> > > tid value before they loop and if the current tid ==
saved_tid
> > > then use the current JavaThread* instead of calling
> > > find_JavaThread_from_java_tid() to get the
JavaThread*.
> > >
> > > Dan
> > >
> > > >
> > > > Webrev:
https://cr.openjdk.java.net/~dtitov/8185005/webrev.02/
> > > > Bug:
https://bugs.openjdk.java.net/browse/JDK-8185005
> > > >
> > > > Thanks!
> > > > --Daniil
> > > >
> > > > From: <serguei.spit...@oracle.com>
> > > > Organization: Oracle Corporation
> > > > Date: Friday, June 28, 2019 at 7:56 PM
> > > > To: Daniil Titov <daniil.x.ti...@oracle.com>, OpenJDK Serviceability
<serviceability-dev@openjdk.java.net>, "hotspot-runtime-...@openjdk.java.net"
<hotspot-runtime-...@openjdk.java.net>, "jmx-...@openjdk.java.net" <jmx-...@openjdk.java.net>
> > > > Subject: Re: RFR: 8185005: Improve performance of
ThreadMXBean.getThreadInfo(long ids[], int maxDepth)
> > > >
> > > > Hi Daniil,
> > > >
> > > > I have several quick comments.
> > > >
> > > > The indent in the hotspot c/c++ files has to be 2,
not 4.
> > > >
> > > >
https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/src/hotspot/share/runtime/threadSMR.cpp.frames.html
> > > > 614 JavaThread*
ThreadsList::find_JavaThread_from_java_tid(jlong java_tid) const {
> > > > 615 JavaThread* java_thread =
ThreadTable::find_thread(java_tid);
> > > > 616 if (java_thread == NULL && java_tid ==
PMIMORDIAL_JAVA_TID) {
> > > > 617 // ThreadsSMRSupport::add_thread() is
not called for the primordial
> > > > 618 // thread. Thus, we find this thread
with a linear search and add it
> > > > 619 // to the thread table.
> > > > 620 for (uint i = 0; i < length(); i++) {
> > > > 621 JavaThread* thread = thread_at(i);
> > > > 622 if
(is_valid_java_thread(java_tid,thread)) {
> > > > 623
ThreadTable::add_thread(java_tid, thread);
> > > > 624 return thread;
> > > > 625 }
> > > > 626 }
> > > > 627 } else if (java_thread != NULL &&
is_valid_java_thread(java_tid, java_thread)) {
> > > > 628 return java_thread;
> > > > 629 }
> > > > 630 return NULL;
> > > > 631 }
> > > > 632 bool ThreadsList::is_valid_java_thread(jlong
java_tid, JavaThread* java_thread) {
> > > > 633 oop tobj = java_thread->threadObj();
> > > > 634 // Ignore the thread if it hasn't run
yet, has exited
> > > > 635 // or is starting to exit.
> > > > 636 return (tobj != NULL &&
!java_thread->is_exiting() &&
> > > > 637 java_tid ==
java_lang_Thread::thread_id(tobj));
> > > > 638 }
> > > >
> > > > 615 JavaThread* java_thread =
ThreadTable::find_thread(java_tid);
> > > >
> > > > I'd suggest to rename find_thread() to
find_thread_by_tid().
> > > >
> > > > A space is missed after the comma:
> > > > 622 if (is_valid_java_thread(java_tid,thread)) {
> > > >
> > > > An empty line is needed before L632.
> > > >
> > > > The name 'is_valid_java_thread' looks wrong (or
confusing) to me.
> > > > Something like 'is_alive_java_thread_with_tid()'
would be better.
> > > > It'd better to list parameters in the opposite
order.
> > > >
> > > > The call to is_valid_java_thread() is confusing:
> > > > 627 } else if (java_thread != NULL &&
is_valid_java_thread(java_tid, java_thread)) {
> > > >
> > > > Why would the call
ThreadTable::find_thread(java_tid) return a JavaThread with an unmatched java_tid?
> > > >
> > > >
> > > > Thanks,
> > > > Serguei
> > > >
> > > > On 6/28/19, 9:40 PM, "David Holmes"
<david.hol...@oracle.com> wrote:
> > > >
> > > > Hi Daniil,
> > > >
> > > > The definition and use of this hashtable (yet
another hashtable
> > > > implementation!) will need careful
examination. We have to be concerned
> > > > about the cost of maintaining it when it may
never even be queried. You
> > > > would need to look at footprint cost and
performance impact.
> > > >
> > > > Unfortunately I'm just about to board a plane
and will be out for the
> > > > next few days. I will try to look at this asap
next week, but we will
> > > > need a lot more data on it.
> > > >
> > > > Thanks,
> > > > David
> > > >
> > > > On 6/28/19 3:31 PM, Daniil Titov wrote:
> > > > Please review the change that improves performance
of ThreadMXBean MXBean methods returning the
> > > > information for specific threads. The change
introduces the thread table that uses ConcurrentHashTable
> > > > to store one-to-one the mapping between the thread
ids and JavaThread objects and replaces the linear
> > > > search over the thread list in
ThreadsList::find_JavaThread_from_java_tid(jlong tid) method with the lookup
> > > > in the thread table.
> > > >
> > > > Testing: Mach5 tier1,tier2 and tier3 tests
successfully passed.
> > > >
> > > > Webrev:
https://cr.openjdk.java.net/~dtitov/8185005/webrev.01/
> > > > Bug:
https://bugs.openjdk.java.net/browse/JDK-8185005
> > > >
> > > > Thanks!
> > > >
> > > > Best regards,
> > > > Daniil
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
