Hi Daniil,
Sorry I found it harder to get to this this week than I would have
hoped, so I've asked a couple of other runtime folk to please take a
look while I'm on vacation. I do have some comments though.
First, you've based this off the ResolvedMethodTable code and it isn't
clear to me that everything in that code necessarily maps to this code.
For example:
68 static uintx get_hash(Value const& value, bool* is_dead) {
69 *is_dead = false;
...
167 bool equals(ThreadTableEntry **value, bool* is_dead) {
168 *is_dead = false;
"is_dead" is not relevant for this code and should be deleted.
Then I'm not at all clear about getting the serviceThread to
resize/rehash the table. Is there a specific reason to do that or did
you just copy what is done for the ResolvedMethodTable? The usage
constraints of that table may be different to this one and require using
the serviceThread where here we may not need to.
The initialization in universe_init() may not be right for the
ThreadsTable, which is logically encapsulated by ThreadsSMRSupport.
Overall there is complexity in ResolvedMethodTable code that I don't
grok and it isn't obvious to me that it is all needed here.
Further, the ConcurrentHashTable was only added in Java 11 so this will
still need an alternate implementation - as per the bug report - to
backport to Java 8.
The is_valid_java_thread you added to ThreadsList isn't really needed. I
know you've copied the core of that logic from the linear search code,
but it really doesn't apply when using the table given the way you keep
the table up to date. If you find the JavaThread using a given tid then
that is the JavaThread.
There's a typo PMIMORDIAL_JAVA_TID.
I'm unclear about the tid==1 handling, you say
"ThreadsSMRSupport::add_thread() is not called for the primordial thread"
but the main thread does have this called via Threads:add just like
every other created or attached thread. And note this isn't generally
the "primordial thread" (which is the initial thread of a process) but
just the "main" thread used to load the JVM.
Overall I'm concerned about the duplication/overlap that now exists
between the ThreadsList and the ThreadsTable. Maybe it is unavoidable to
get the hashed lookup, or perhaps there is some way to get the desired
functionality without the overlap? I'm hoping Dan will be able to chime
in on that (ideally Robbin too but he is away this month.).
And of course we still need to check overall footprint and performance
impact. (E.g. if we have large numbers of threads might the rehash cause
observable pauses in the other cleanup activities that the service
thread does?).
Thanks,
David
-----
On 29/06/2019 2:39 pm, David Holmes 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 28/06/2019 6: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
