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

Small improvements to Timer #39510

Merged
merged 3 commits into from
Feb 5, 2021
Merged

Small improvements to Timer #39510

merged 3 commits into from
Feb 5, 2021

Conversation

vchuravy
Copy link
Member

@vchuravy vchuravy commented Feb 3, 2021

After much frustration today debugging timer code it boiled down to two things:

  1. When interval is to small we round to 0 and accidentially create a one-shot timer
  2. Errors in the callback are silently dropped.

@vchuravy vchuravy requested a review from vtjnash February 3, 2021 22:11
@vchuravy vchuravy changed the title Small improvements Small improvements to Timer Feb 3, 2021
test/channels.jl Outdated Show resolved Hide resolved
test/channels.jl Outdated Show resolved Hide resolved
Jameson Nash and others added 3 commits February 4, 2021 15:59
This is a performance/memory optimization: previously, it would start
running at an indeterminate time between when it was defined and when it
was needed, possibly wasting memory until then.
@vchuravy
Copy link
Member Author

vchuravy commented Feb 4, 2021

Should I drop 70d5ca8 in favor of #39518?

@JeffBezanson
Copy link
Member

I think this is fine for now.

@JeffBezanson JeffBezanson merged commit af353ec into master Feb 5, 2021
@JeffBezanson JeffBezanson deleted the vc/timer branch February 5, 2021 23:25
@@ -72,7 +75,7 @@ mutable struct Timer
timeout ≥ 0 || throw(ArgumentError("timer cannot have negative timeout of $timeout seconds"))
interval ≥ 0 || throw(ArgumentError("timer cannot have negative repeat interval of $interval seconds"))
timeout = UInt64(round(timeout * 1000)) + 1
interval = UInt64(round(interval * 1000))
interval = UInt64(ceil(interval * 1000))
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to make sure that interval is at least 1 if it is greater than zero on the way in?

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* delay launching Task until just before it is needed

This is a performance/memory optimization: previously, it would start
running at an indeterminate time between when it was defined and when it
was needed, possibly wasting memory until then.

* Timer: don't create a one-shot timer

* Timer: Don't fail silently when callback fails

Co-authored-by: Jameson Nash <jameson@juliacomputing.com>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* delay launching Task until just before it is needed

This is a performance/memory optimization: previously, it would start
running at an indeterminate time between when it was defined and when it
was needed, possibly wasting memory until then.

* Timer: don't create a one-shot timer

* Timer: Don't fail silently when callback fails

Co-authored-by: Jameson Nash <jameson@juliacomputing.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants