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

[serguei.spitsyn at oracle.com]

Hi Dan,

On 9/20/19 18:11, Daniel D. Daugherty wrote:
> 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.

It is hard to follow all moves on this email thread. :)

Thanks,
Serguei