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

[Daniel D. Daugherty]

On 9/20/19 9:07 PM, Daniel D. Daugherty wrote:
> On 9/20/19 7:15 PM, serguei.spitsyn at oracle.com wrote:
>> 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.

I should have read your comment more carefully...

Yes, you are right about the early cut out on L97-100 causing
the for-loop to be skipped by any thread that saw (!_is_initialized)
and was blocked on the ThreadTableCreate_lock.

So my response below is wrong...

Sorry about the noise.

Dan

>
> The ThreadTableCreate_lock is only held from here:
>
>   L95:     {
>   L96:       MutexLocker ml(ThreadTableCreate_lock);
>
> to here:
>
>   L103:       _is_initialized = true;
>   L104:     }
>
> and the for-loop starts here:
>
>   L105:     for (uint i = 0; i < threads->length(); i++) {
>
> so the ThreadTable_Create_lock does not linearize the
> for-loop.
>
> I'm quoting webrev.07... which I believe is the latest.
>
> Dan
>
>
>>
>> . . .
>>> 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
>>>
>>
>