RFR: 8231666: ThreadIdTable::grow() invokes invalid thread transition

[Robbin Ehn]

Hi, since holding the Threads_lock while growing can block out safepoints for a
longer period, I would suggest just skip growing when holding Threads_lock.
E.g. return before creating the GrowTask.

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