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

[Daniel D. Daugherty]

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.

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