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

[David Holmes]

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