Fix DefaultExecutor not being able to exit #1876
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
elizarov
suggested changes
Mar 24, 2020
There was a problem hiding this comment.
While we are here, fixing this code, I'd also suggest to make the following performance improvement.
See, right now looping for events works like this:
- The next event is taken from the queue and executed.
nextTimeis called to see when the event after this happens (and the queue is analyzed for that). Assume we have an event in the queue, so- We immediately go to 1, where we look at the queue again to remove this event.
It seems inefficient to check queues twice in a row. Instead, I'd suggest to always return 0 from processNextEvent when even was just executed and return nextTime only when no event was executed.
| @@ -249,9 +258,15 @@ internal abstract class EventLoopImplBase: EventLoopImplPlatform(), Delay { | |||
| } | |||
| } | |||
|
|
|||
| override fun processNextEvent(): Long { | |||
| override fun popNextTask(onlyUnconfined: Boolean): Runnable? { | |||
There was a problem hiding this comment.
I cannot find a place where this method if called with onlyUnconfined = true.
|
If we do this optimization, then the decomposition of Should I keep the decomposition or should I revert it and instead implement the small change to the logic of |
It would be also great to avoid decomposition. It will make the code easier to understand. |
This test could in theory already fail on the second `checkNotEmpty`: after the first `checkNotEmpty` has passed, first, the ticker channel awakens to produce a new element, and then the main thread awakens to check if it's there. However, if the ticker channel is sufficiently slowed down, it may not produce the element in time for the main thread to find it. After introducing the change that allows the worker thread in `DefaultExecutor` to shut down, the initial delay of 2500 ms is sufficient for the worker to shut down (which, by default, happens after 1000 ms of inactivity), and then the aforementioned race condition worsens: additional time is required to launch a new worker thread and it's much easier to miss the deadline. Now, the delays are much shorter, meaning that the worker thread is never inactive long enough to shut down.
Also adds the performance optimization. See the discussion on the PR.
elizarov
suggested changes
Mar 26, 2020
qwwdfsad
pushed a commit
that referenced
this pull request
Apr 24, 2020
* Fix DefaultExecutor not being able to exit. * Also adds the performance optimization. See the discussion on the PR. * Add a stress test for the DefaultExecutor worker shutting down. * Make `testDelayChannelBackpressure2` not fail: This test could in theory already fail on the second `checkNotEmpty`: after the first `checkNotEmpty` has passed, first, the ticker channel awakens to produce a new element, and then the main thread awakens to check if it's there. However, if the ticker channel is sufficiently slowed down, it may not produce the element in time for the main thread to find it. After introducing the change that allows the worker thread in `DefaultExecutor` to shut down, the initial delay of 2500 ms is sufficient for the worker to shut down (which, by default, happens after 1000 ms of inactivity), and then the aforementioned race condition worsens: additional time is required to launch a new worker thread and it's much easier to miss the deadline. Now, the delays are much shorter, meaning that the worker thread is never inactive long enough to shut down. Fixes #856
recheej
pushed a commit
to recheej/kotlinx.coroutines
that referenced
this pull request
Dec 28, 2020
* Fix DefaultExecutor not being able to exit. * Also adds the performance optimization. See the discussion on the PR. * Add a stress test for the DefaultExecutor worker shutting down. * Make `testDelayChannelBackpressure2` not fail: This test could in theory already fail on the second `checkNotEmpty`: after the first `checkNotEmpty` has passed, first, the ticker channel awakens to produce a new element, and then the main thread awakens to check if it's there. However, if the ticker channel is sufficiently slowed down, it may not produce the element in time for the main thread to find it. After introducing the change that allows the worker thread in `DefaultExecutor` to shut down, the initial delay of 2500 ms is sufficient for the worker to shut down (which, by default, happens after 1000 ms of inactivity), and then the aforementioned race condition worsens: additional time is required to launch a new worker thread and it's much easier to miss the deadline. Now, the delays are much shorter, meaning that the worker thread is never inactive long enough to shut down. Fixes Kotlin#856
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Solves #856.