RFR: 8185005: Improve performance of ThreadMXBean.getThreadInfo(long ids[], int maxDepth)

[David Holmes]

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