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

[serguei.spitsyn at oracle.com]

Hi David,

I do not want to insist in the area (Threads_lock use) where you have 
much more expertise.
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.

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

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