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

Use Dispatchers.Main as default delay source where applicable #2974

Merged
merged 2 commits into from Nov 17, 2021

Conversation

qwwdfsad
Copy link
Member

It reduces the number of redundant threads in the system and makes time source predictable across Android/JavaFx applications

Fixes #2972

Copy link
Collaborator

@dkhalanskyjb dkhalanskyjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invokeOnTimeout docs state:

This implementation uses a built-in single-threaded scheduled executor service.

First, it's probably not true at all, given that there are no qualifiers like "on the JVM", and second, it should probably now mention that DefaultDelay can be configured.

* When we already are working with UI and Main threads, it makes
* no sense to create a separate thread with timer that cannot be controller
* by the UI runtime.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, there are reasons not to tie the delays to the UI: just look at video games that tie their main loops to the frame rate and exhibit surprising behavior in physics, audio, scripts etc. when just the video card is subpar and fails to draw the image in time, rendering the game unplayable rather than just choppy.

I don't know how Android video games are implemented and whether it's usual for them to flood the event loop so that the UI struggles, but if so, then now much of the app would struggle as well, also potentially exacerbating the issue.

I looked at the Dispatchers.Main implementations that we have, just to see if they're suitable as the default delays.

Android uses https://developer.android.com/reference/android/os/Handler#postDelayed(java.lang.Runnable,%20java.lang.Object,%20long)

The time-base is SystemClock.uptimeMillis().

Millisecond precision looks fine, but googling, I find concerns about postDelayed being too slow for anything other than UI events:

JavaFX isn't much better in this regard (https://docs.oracle.com/javase/8/javafx/api/javafx/animation/Timeline.html):

Timeline processes individual KeyFrame at or after specified time interval elapsed, it does not guarantee the timing when KeyFrame is processed.

Doesn't sound good at all to me as far as guarantees go.

I didn't find much in the Swing documentation regarding its timers, but people do complain about its Timer being too slow and UI-dependent also:

To sum it up, I'm very much not comfortable with this change being opt-out rather than opt-in. Updating the coroutines only to find that having a suboptimal UI now causes other tasks in the app to become slower doesn't sound like an easy-to-debug situation.

Maybe I misunderstand the scope of this change somehow and DefaultDelay isn't really relied on a lot in practice?

internal actual val DefaultDelay: Delay = DefaultExecutor
internal actual val DefaultDelay: Delay = initializeDefaultDelay()

private val defaultMainDelayOptIn = systemProp("kotlinx.coroutines.main.delay", true)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be documented at least somewhere.

Copy link
Member Author

@qwwdfsad qwwdfsad Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can mention it in the original issue and in the changelog. Does it work for you?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, we can document Delay itself, but it doesn't make it to the Dokka and is internal interface, so it's unlikely someone will read it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That depends. Do we plan to remove this opt-in flag in a release or two if no one complains or is it likely that we'll keep it? If it's the latter, then I don't think only documenting a configuration parameter in a changelog (or a closed issue) is sufficient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see any urge in removing it (e.g. we're still preserving flag for fast service loader), its maintenance has basically zero cost

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A suggestion then: maybe later, outside of this pull request, we should document in the README (either the top-level one or for the core module) all the flags that our code non-transitively recognizes (or even transitively, if viable).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would advise against it. All the options are intended to be user-facing features, but rather emergency brakes when everything went terribly wrong. Some of the flags (e.g. in the scheduler) can only be used for specific performance scenarios, and the overall behaviour with an arbitrary set of options is not well-defined.

It reduces the number of redundant threads in the system and makes time source predictable across Android/JavaFx applications

Fixes #2972
@qwwdfsad qwwdfsad merged commit e3cd41b into develop Nov 17, 2021
@qwwdfsad qwwdfsad deleted the main-friendly-default-delay branch November 17, 2021 12:48
yorickhenning pushed a commit to yorickhenning/kotlinx.coroutines that referenced this pull request Jan 28, 2022
…#2974)

* Use Dispatchers.Main as default delay source where applicable

It reduces the number of redundant threads in the system and makes time source predictable across Android/JavaFx applications

Fixes Kotlin#2972
pablobaxter pushed a commit to pablobaxter/kotlinx.coroutines that referenced this pull request Sep 14, 2022
…#2974)

* Use Dispatchers.Main as default delay source where applicable

It reduces the number of redundant threads in the system and makes time source predictable across Android/JavaFx applications

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

Successfully merging this pull request may close these issues.

None yet

2 participants