RFR: avoid extra park of vthread when using fixed thread pool

[Alan Bateman]

On Thu, 26 Aug 2021 08:47:10 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> The test case of ParkWithFixedThreadPool.java create 300 virtual threads, each vthread(exclude latest vthread) park itself and unpark previous vthread. The expected result is any vthread can finish.
>> 
>> Running this test case in slowdebug and the test will random hang.
>> 
>> There are three vthreads which are vt-1, vt-2, vt-3;
>> (1) vt-1 take the ReentrantLock, and vt-2 try to unpark vt-1, and vt-2 fail to get ReentrantLock so it park itself( the call stack is shown below); 
>> (2)vt-3 unpark vt-2, and vt-2 try to get ReentrantLock again, but the ReentrantLock is still owned by vt-1, vt-2 park itself again;
>> (3)vt-1 release the ReentrantLock and unpark vt-2, vt-2 park itself at ParkWithFixedThreadPool.java:55, and it will never unpark.(because the unpark from vt-3 has consumed)
>> 
>> The reason is scheduler.execute() will try to alloc a Reentrant lock when using fix thread pool, the call stack is like:
>>     at java.lang.VirtualThread.tryPark(VirtualThread.java:472)
>>     at java.lang.VirtualThread.park(VirtualThread.java:424)
>>     at java.lang.System$2.parkVirtualThread(System.java:1279)
>>     at sun.misc.VirtualThreads.park(VirtualThreads.java:56)
>>     at java.util.concurrent.locks.LockSupport.park(LockSupport.java:183)
>>     at java.util.concurrent.locks.AbstractQueuedSynchronizer.parkAndCheckInterrupt(AbstractQueuedSynchronizer.java:836)
>>     at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquireQueued(AbstractQueuedSynchronizer.java:870)
>>     at java.util.concurrent.locks.AbstractQueuedSynchronizer.acquire(AbstractQueuedSynchronizer.java:1199)
>>     at java.util.concurrent.locks.ReentrantLock$NonfairSync.lock(ReentrantLock.java:209)
>>     at java.util.concurrent.locks.ReentrantLock.lock(ReentrantLock.java:285)
>>     at java.util.concurrent.LinkedBlockingQueue.offer(LinkedBlockingQueue.java:418)
>>     at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1371)
>>     at java.lang.VirtualThread.unpark(VirtualThread.java:502)
>>     at java.lang.System$2.unparkVirtualThread(System.java:1287)
>>     at sun.misc.VirtualThreads.unpark(VirtualThreads.java:70)
>>     at ParkWithFixedThreadPool$1.run(ParkWithFixedThreadPool.java:51)
>> 
>> 
>> The solution is switch back to carrier thread before call scheduler.execute();
>
> Thanks for this issue. I think we have a regression here as unpark used to switch to the carrier to avoid parking on the virtual thread. Can you try this:
> 
> 
> --- a/src/java.base/share/classes/java/lang/VirtualThread.java
> +++ b/src/java.base/share/classes/java/lang/VirtualThread.java
> @@ -664,7 +664,19 @@ class VirtualThread extends Thread {
>          if (!getAndSetParkPermit(true) && Thread.currentThread() != this) {
>              int s = state();
>              if (s == PARKED && compareAndSetState(PARKED, RUNNABLE)) {
> -                submitRunContinuation(tryPush);
> +
> +                if (Thread.currentThread() instanceof VirtualThread vthread) {
> +                    Thread carrier = vthread.carrierThread;
> +                    carrier.setCurrentThread(carrier);
> +                    try {
> +                        submitRunContinuation(tryPush);
> +                    } finally {
> +                        carrier.setCurrentThread(vthread);
> +                    }
> +                } else {
> +                    submitRunContinuation(tryPush);
> +                }
> +
>              } else if (s == PINNED) {
>                  // signal pinned thread so that it continues
>                  final ReentrantLock lock = getLock();
> 
> 
> For the test, would you mind changing it to use try-with-resources  so that it will shutdown the thread pool even if there is an exception. 
> 
> As we're here, I should mention that it is still "TBD" if we will keep the support for custom schedulers in the first version. As you found, custom schedulers mean running arbitrary code at potentially critical points (like unparking). It might be that we need to give custom schedulers further thought before deciding whether to expose or not.

> @AlanBateman Thanks for your review and answer.
> I have changed this PR on your suggestions.

Thanks for the update. I'd like to do a bit more testing with this as I suspect it may require an update to at least one of the tests that exercises custom schedulers. Will get back to you soon on this.

-------------

PR: https://git.openjdk.java.net/loom/pull/59