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

[serguei.spitsyn at oracle.com]

On 9/20/19 23:53, David Holmes wrote:
> 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.

I've figured the same.

> But I still prefer on principle to not hold nested locks if it is not 
> necessary.

Simplicity and conservatism rule! :)

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

Agreed.
It is my usual approach too (which I've just tried to break). :)


Thanks,
Serguei

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