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

[David Holmes]

On 21/09/2019 4:21 pm, serguei.spitsyn at oracle.com wrote:
> Hi David,
> 
> I do not want to insist in the area (Threads_lock use) where you have 
> much more expertise.

It's more conservatism than expertise :) I had a look through and there 
are not too many usages of the Threads_lock in the M&M code and they do 
appear safe. But I still prefer on principle to not hold nested locks if 
it is not necessary.

> Also, I agreed that Daniil's webrev.07 version is correct and less risky.
> The only my concern was about possible performance impact of linear 
> searches
> from the ThreadsList::find_JavaThread_from_java_tid() calls that can be 
> executed
> concurrently with the ThreadTable::lazy_initialize() if the ThreadsList 
> is big.
> But it should not be that big issue.

I think we can waste a lot of time wondering about what kinds of 
scenarios perform better or worse with which variant of the code. You 
will always be able to create a microbenchmark for which a particular 
code pattern works best. So I'd say stick to the simplest and least 
risky code and worry about performance issues like this if, or when, 
they turn up.

> So, I'm okay with the webrev.07 modulo ThreadTable renaming.

Okay.

Thanks,
David

> Thanks,
> Serguei
> 
> 
> On 9/20/19 19:20, David Holmes wrote:
>> Hi Serguei,
>>
>> On 21/09/2019 9:50 am, serguei.spitsyn at oracle.com wrote:
>>> Hi Daniil,
>>>
>>> Yes, the Threads_lock is still needed around thread->is_exiting() 
>>> check and add_thread().
>>>
>>>> And if we try to use 2 locks, ThreadTableCreate_lock as in your snippet
>>>> and then the nested Threads_lock around thread->is_exiting() and
>>>> add_thread(java_tid, thread) lines then it will not work since the rank
>>>> of Threads_lock  is higher than the rank of ThreadTableCreate_lock.
>>>
>>> The ThreadTableCreate_lock is only used in the lazy_initialize() 
>>> function.
>>> It looks safe to adjust the ThreadTableCreate_lock to fix the above 
>>> problem.
>>> Then the approach below will work as needed.
>>>
>>>   void ThreadTable::lazy_initialize(const ThreadsList *threads) {
>>>      if (_is_initialized) {
>>>        return;
>>>      }
>>>      MutexLocker ml(ThreadTableCreate_lock);
>>>      if (_is_initialized) {
>>>        // There is no obvious benefits in allowing the thread table
>>>        // being concurrently populated during the initalization.
>>>        return;
>>>      }
>>>      create_table(threads->length());
>>>      _is_initialized = true;
>>>
>>>      for (uint i = 0; i < threads->length(); i++) {
>>>        JavaThread* thread = threads->thread_at(i);
>>>        oop tobj = thread->threadObj();
>>>        if (tobj != NULL) {
>>>          jlong java_tid = java_lang_Thread::thread_id(tobj);
>>>          MutexLocker ml(Threads_lock);
>>>          if (!thread->is_exiting()) {
>>>            // Must be inside the lock to ensure that we don't add the 
>>> thread to the table
>>>            // that has just passed the removal point in 
>>> ThreadsSMRSupport::remove_thread()
>>>            add_thread(java_tid, thread);
>>>          }
>>>        }
>>>      }
>>>    }
>>
>> I don't like holding a completely unrelated lock, whilst holding the 
>> Threads_lock, particularly as it seems unnecessary for correctness. We 
>> have to be certain that no M&M operation that holds the Threads_lock 
>> can try to initialize the thread table. Testing can only prove the 
>> presence of that problem, not the absence.
>>
>> I think Daniil's webrev.07 version is correct and less risky than what 
>> you propose. Sorry.
>>
>> David
>> -----