Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yield optimizations don't work correctly #4248

Open
dkhalanskyjb opened this issue Oct 11, 2024 · 1 comment
Open

yield optimizations don't work correctly #4248

dkhalanskyjb opened this issue Oct 11, 2024 · 1 comment
Labels

Comments

@dkhalanskyjb
Copy link
Collaborator

dkhalanskyjb commented Oct 11, 2024

Describe the bug

yield() introduces special behavior for optimization: instead of calling dispatch, it calls a specialized dispatchYield function. In theory, that function allows for some optimization: we know we're already executing a coroutine on the correct thread, so we can skip yield altogether if there are no other coroutines scheduled; we know the coroutine is going to suspend immediately, so we don't need to spawn a new worker to accommodate it; and so on.

This theory breaks because of CoroutineStart.UNDISPATCH: if we call yield() the first thing in an undispatched coroutine, none of this is true.

Provide a Reproducer

Currently, because this optimization is very selectively applied, only Dispatchers.Default suffers from this, and because of an internal check, only when the new coroutine is started from a kotlinx-coroutines-controlled thread.

        withContext(Dispatchers.Default) {
            println("Starting on ${Thread.currentThread()} at ${System.currentTimeMillis()}")
            launch(start = CoroutineStart.UNDISPATCHED) {
                println("New coroutine on ${Thread.currentThread()} at ${System.currentTimeMillis()}")
                yield()
                println("Finished yielding on ${Thread.currentThread()} at ${System.currentTimeMillis()}")
            }
            println("Back to the old coroutine on ${Thread.currentThread()} at ${System.currentTimeMillis()}")
            Thread.sleep(5000)
            println("Slept for five seconds on ${Thread.currentThread()} at ${System.currentTimeMillis()}")
        }

Prints

Starting on Thread[DefaultDispatcher-worker-2 @coroutine#1,5,main] at 1728644869811
New coroutine on Thread[DefaultDispatcher-worker-2 @coroutine#2,5,main] at 1728644869812
Back to the old coroutine on Thread[DefaultDispatcher-worker-2 @coroutine#1,5,main] at 1728644869813
Slept for five seconds on Thread[DefaultDispatcher-worker-2 @coroutine#1,5,main] at 1728644874813
Finished yielding on Thread[DefaultDispatcher-worker-2 @coroutine#2,5,main] at 1728644874814

The new thread that was supposed to be used for the new coroutine after it's yielded does not actually wake up, causing an unjustified delay.

@dkhalanskyjb
Copy link
Collaborator Author

Coroutines themselves don't know whether they were started in an undispatched manner, and so yield can not disable the optimization in these special cases. We can teach coroutines that, but this requires that we modify our JobSupport state machine significantly. A less nice solution is to just remove the optimization.

qwwdfsad added a commit that referenced this issue Oct 21, 2024
* Always unpark a worker to avoid potential starvation in cases when coroutine was launched via UNDISPATCHED mechanism

Fixes #4248
qwwdfsad added a commit that referenced this issue Oct 21, 2024
* Treat dispatchYield as regular dispatch in CoroutineScheduler
* Always unpark a worker to avoid potential starvation in cases when coroutine was launched via UNDISPATCHED mechanism

Fixes #4248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant