Hi David and Robbin,
Could we consider making the ServiceThread responsible for the ThreadIdTable
resizing in the similar way how
it works for StringTable and ResolvedMethodTable, rather than having
ThreadIdTable::add() method calling ThreadIdTable::grow()?
As I understand It should solve the current issue and address the concern
that the doing the resizing could be a relatively long and
doing it without polling for safepoints or while the holding Threads_lock is
not desirable.
Thank you,
Daniil
On 10/2/19, 6:25 AM, "David Holmes" <david.hol...@oracle.com> wrote:
Hi Robbin,
On 2/10/2019 7:58 pm, Robbin Ehn wrote:
> Hi David,
>
>> What if the table is full and must be grown?
>
> The table uses chaining, it just means load factor tip over what is
> considered a good backing array size.
Coleen raised a good question in a separate discussion, which made me
realize that once the table has been initially populated all subsequent
additions, and hence all subsequent calls to grow() always happen with
the Threads_lock held. So we can't just defer the grow().
>> That aside, I'd like to know how expensive it is to grow this table.
>> What are we talking about here?
>
> We use global counter which on write_synchronize must scan all
> threads to make sure they have seen the update (there some
> optimization to avoid it if there is no readers at all). Since this
> table contains the threads, we get double penalized, for each new
> thread the synchronization cost increase AND the number of items.
>
> With concurrent reads you still need many thousands of threads, but
> I think I saw someone mentioning 100k threads, assuming concurrent
> queries the resize can take hundreds of ms to finish. Note that reads
> and inserts still in operate roughly at the same speed while
> resizing. So a longer resize is only problematic if we do not
> respect safepoints.
I think if anything were capable of running 100K threads we would be
hitting far worse scalability bottlenecks than this. But this does seem
problematic.
Thanks,
David
-----
> Thanks, Robbin
>
>>
>> David
>>
>>> /Robbin
>>>
>>> On 2019-10-02 08:46, David Holmes wrote:
>>>> Hi Daniil,
>>>>
>>>> On 2/10/2019 4:13 pm, Daniil Titov wrote:
>>>>> Please review a change that fixes the issue. The problem here is
>>>>> that that the thread is added to the ThreadIdTable (introduced in
>>>>> [3]) while the Threads_lock is held by
>>>>> JVM_StartThread. When new thread is added to the thread table the
>>>>> table checks if its load factor is greater than required and if so
>>>>> it grows itself while polling for safepoints.
>>>>> After changes [4] an attempt to block the thread while holding the
>>>>> Threads_lock results in assertion in
>>>>> Thread::check_possible_safepoint().
>>>>>
>>>>> The fix proposed by David Holmes ( thank you, David!) is to skip
>>>>> the ThreadBlockInVM inside ThreadIdTable::grow() method if the
>>>>> current thread owns the Threads_lock.
>>>>
>>>> Sorry but looking at the fix in context now I think it would be
>>>> better to do this:
>>>>
>>>> while (gt.do_task(jt)) {
>>>> if (Threads_lock->owner() == jt) {
>>>> gt.pause(jt);
>>>> ThreadBlockInVM tbivm(jt);
>>>> gt.cont(jt);
>>>> }
>>>> }
>>>>
>>>> This way we don't waste time with the pause/cont when there's no
>>>> safepoint pause going to happen - and the owner() check is quicker
>>>> than owned_by_self(). That partially addresses a general concern I
>>>> have about how long it may take to grow the table, as we are
>>>> deferring safepoints until it is complete in this JVM_StartThread
>>>> usecase.
>>>>
>>>> In the test you don't need all of:
>>>>
>>>> 32 * @run clean ThreadStartTest
>>>> 33 * @run build ThreadStartTest
>>>> 34 * @run main ThreadStartTest
>>>>
>>>> just the last @run suffices to build and run the test.
>>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> Testing : Mach 5 tier1 and tier2 completed successfully, tier3 is
>>>>> in progress.
>>>>>
>>>>> [1] Webrev: http://cr.openjdk.java.net/~dtitov/8231666/webrev.01/
>>>>> [2] Bug: https://bugs.openjdk.java.net/browse/JDK-8231666
>>>>> [3] https://bugs.openjdk.java.net/browse/JDK-8185005
>>>>> [4] https://bugs.openjdk.java.net/browse/JDK-8184732
>>>>>
>>>>> Best regards,
>>>>> Danill
>>>>>
>>>>>
