FIX/ENH: macos: dispatch timer tasks asynchronously to the main loop #29030
Conversation
This file contains hidden or 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
Previously, the timers were dependent on the length of time it took for the timer callback to execute. This dispatches the callback to the task queue to avoid synchronously waiting on long-running callback tasks.
greglucas
commented
Oct 28, 2024
| // we shouldn't do it ourselves when the object is deleted. | ||
| self->timer = NULL; | ||
| } | ||
| self->timer = [NSTimer scheduledTimerWithTimeInterval: interval |
There was a problem hiding this comment.
This automatically schedules the timer on the main runloop for us.
https://developer.apple.com/documentation/foundation/timer/2091889-scheduledtimer
| self->timer = [NSTimer scheduledTimerWithTimeInterval: interval | ||
| repeats: !single | ||
| block: ^(NSTimer *timer) { | ||
| dispatch_async(dispatch_get_main_queue(), ^{ |
There was a problem hiding this comment.
This is the major change here. We are dispatching our event asynchronously (so as to not sequentially depend on the callback processing time), but putting it onto the main thread's queue.
|
closing in favor of doing this all in a consolidated PR: #29062 |
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.
PR summary
Previously, the timers were dependent on the length of time it took for the timer callback to execute. This dispatches the callback to the main thread's task queue to avoid synchronously waiting on long-running callback tasks.
Note that I didn't add any tests here because there is inconsistency between backends, so other backends fail for these assertions currently. I've opened up an issue to discuss that: #29029
On main, this script prints every 2 seconds
callback time + timer interval, with this PR it prints every secondmax(callback time, timer interval).PR checklist