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