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

[serguei.spitsyn at oracle.com]

Hi Dan,

Please, find a minor correction below.


On 9/20/19 2:59 PM, Daniel D. Daugherty wrote:
> Daniil,
>
> Thanks for sticking with this project through the many versions.
> Sorry this review is late...
>
>
> On 9/19/19 8:30 PM, Daniil Titov wrote:
>> Hi David and Serguei,
>>
>> Please review new version of the fix that includes the changes 
>> Serguei suggested:
>>   1. If racing threads initialize the thread table only one of these 
>> threads will populate the table with the threads from the thread list
>>   2. The code that adds the thread to the tread table is put inside 
>> Threads_lock to ensure that we cannot accidentally add the thread
>>       that has just passed the removal point in 
>> ThreadsSMRSupport::remove_thread()
>>
>> The changes are in ThreadTable::lazy_initialize() method only.
>>
>> Testing:  Mach5 tier1, tier2, tier3, tier4, and tier5 tests 
>> successfully passed.
>>
>> Webrev: https://cr.openjdk.java.net/~dtitov/8185005/webrev.07/
. . .
> L93: void ThreadTable::lazy_initialize(const ThreadsList *threads) {
>         Re: discussion about lazy_initialize() racing with
>             ThreadsList::find_JavaThread_from_java_tid()
>
>         There's a couple of aspects to these two pieces of code racing
>         with each other and racing with new thread creation. Racing with
>         new thread creation is the easy one:
>
>           If a new thread isn't added to the ThreadTable by
>           ThreadsSMRSupport::add_thread() calling 
> ThreadTable::add_thread(),
>           then the point in the future where someone calls
>           find_JavaThread_from_java_tid() will add it to the table due to
>           the linear search when ThreadTable::find_thread_by_tid()
>           returns NULL.
>
>        As for multi-threads calling 
> ThreadsList::find_JavaThread_from_java_tid()
>        at the same time which results in multi-threads in 
> lazy_initialize()
>        at the same time...
>
>        - ThreadTable creation will be linear due to 
> ThreadTableCreate_lock.
>          After _is_initialized is set to true, then no more callers to
>          lazy_initialize() will be in the "if (!_is_initialized)" block.
>        - Once the ThreadTable is created, then multi-threads can be
>          executing the for-loop to add their ThreadsList entries to
>          the ThreadTable.

I guess, there is a confusion here.
The lines 97-101 were recently added into the latest webrev:

   93 void ThreadTable::lazy_initialize(const ThreadsList *threads) {
   94   if (!_is_initialized) {
   95     {
   96       MutexLocker ml(ThreadTableCreate_lock);
   97       if (_is_initialized) {
   98         // There is no obvious benefits in allowing the thread table
   99         // being concurently populated during the initalization.
  100         return;
  101       }

It prevents multi-threads executing the for-loop to add their
ThreadsList entries to the ThreadTable.
Instead, these threads may not find the requested threads in the
ThreadTable and so, will start linear search in their ThreadsList
to add them into the ThreadTable.

. . .
> Short version: Thumbs up.
>
> Longer version: I don't think I've spotted anything other than nits here.
> Mostly I've just looked for multi-threaded races, proper usage of the
> Thread-SMR stuff, and minimal impact in the case where the new
> ThreadsTable is never needed.
>
> Dan
>
> P.S.
> ThreadTable is a bit of misnomer. What you really have here is
> a ThreadIdTable, but I'm really late to the code review flow
> with that comment...

Agreed, ThreadIdTable is better name for this table.

Thanks,
Serguei


>
>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8185005
>>
>> Thank you!
>> --Daniil
>