-
-
Notifications
You must be signed in to change notification settings - Fork 57
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
refactor: leading updates in timers #955
Conversation
Important Auto Review SkippedReview was skipped due to path filters Files ignored due to path filters (5)
You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
ac48779
to
54f4693
Compare
I think this is a good solution to update on the second rollover we could look in to the scheduling, but maybe it is more complexity than it is worth (if it's easy more precision is always nice). increasing notification rate seams "dangerous" at least as a default, but exposing the setting could be nice (and clamp it to some sane values) |
54f4693
to
3698241
Compare
Agreed. We would need some time to check that there are no unintended side effects. |
This PR offers a solution to improve the described in #940
The existing implementation assumes the following:
end
which are scheduled for extra accuracyFrom previous conversations, we felt this was sufficient, but it is understandable that it may cause suspicion and loss of confidence from users side.
The proposed solution implements changes so that we have a leading update on clock and timer.
ie: we update the clock on the turn of the second, not once a second
See below the rate of updates in websocket. The clock message comes within 32ms after the second turn.
I believe we can further improve this by
a) schedule individual updates instead of scheduling the interval update
b) increasing the standard refresh rate (currently 32ms)
However I am unsure if the changes are necessary.
The refresh rate is currently a setting (which is kept away from the users). We could also expose this as an application setting which I am unsure if it is a good idea.