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

Add globally registered timers #135

Merged
merged 5 commits into from
Sep 22, 2021
Merged

Add globally registered timers #135

merged 5 commits into from
Sep 22, 2021

Conversation

morris25
Copy link
Contributor

@morris25 morris25 commented Aug 25, 2021

Closes #134.

Adds get_timer function to register and retrieve timers. This allows users to access timers for different workflows without having to pass them through intermediate packages.

Example use case:

module ReadOnlyDB

using TimerOutputs

function get_data(...)
     db_timer = get_timer("DBAccess")  # Used in all DB packages
     @timeit db_timer "fetch" ...
end
end
using Simulation  # uses PkgA which uses PkgB which uses several DB packages
using TimerOutputs
simulate(...)

print_timer(get_timer("DBAccess"))  # keep track of DB efficiency

src/TimerOutput.jl Outdated Show resolved Hide resolved
@KristofferC
Copy link
Owner

I don't think your example will work:

julia> TimerOutputs._timers
Dict{String, TimerOutput} with 1 entry:
  "Default" =>  ──────────────────────────────────────────────────────────────────…

julia> using ReadOnlyDB

julia> TimerOutputs._timers
Dict{String, TimerOutput} with 1 entry:
  "Default" =>  ──────────────────────────────────────────────────────────────────…

You would need to retrieve the timer in __init__ and assign it somewhere, I think.

@morris25
Copy link
Contributor Author

morris25 commented Aug 30, 2021

I don't think your example will work:

julia> TimerOutputs._timers
Dict{String, TimerOutput} with 1 entry:
  "Default" =>  ──────────────────────────────────────────────────────────────────…

julia> using ReadOnlyDB

julia> TimerOutputs._timers
Dict{String, TimerOutput} with 1 entry:
  "Default" =>  ──────────────────────────────────────────────────────────────────…

You would need to retrieve the timer in __init__ and assign it somewhere, I think.

You're right. I've simplified the example to get the timer locally rather than setting a const. That's what I've been doing in my code anyway. We could add something similar to Memento.register to set registered timers but I'm not sure it's necessary.

Edit: I tried adding a register function and realized that we would run into weird situations when the timer gets disabled if there are multiple copies floating around. It's probably best to only use a timer from _timers locally.

@KristofferC
Copy link
Owner

I think this is ok (still needs some docs in the README). But this shouldn't really be used by packages since it basically puts everything in a shared namespace and if packages use it you might get accidental collisions. This is why I am feeling this is perhaps not the ideal solution to the problem.

@morris25
Copy link
Contributor Author

morris25 commented Sep 7, 2021

I think this is ok (still needs some docs in the README). But this shouldn't really be used by packages since it basically puts everything in a shared namespace and if packages use it you might get accidental collisions. This is why I am feeling this is perhaps not the ideal solution to the problem.

Yeah, this solution is meant for really limited use. I basically want to keep a human-readable trace of the runtimes of a couple components within our system on live runs. As is stands, the current solution good enough for the situation I need it for which is essentially a couple named default timers.

If you have any ideas for alternate solutions to the problem I'd be happy to try them out! I'll add some documentation in the meantime.

@fredrikekre
Copy link
Contributor

Why not just define the named timers as global variables?

@KristofferC
Copy link
Owner

Presumably because you want to share them between multiple packages so one uses TimerOutputs.jl as a "storage" because all packages have access to it.

@morris25
Copy link
Contributor Author

morris25 commented Sep 8, 2021

We have a system that uses ~300 packages and want to keep track of live runtimes for a few components in around 5 of them for business reasons. Our POC uses the default timer but that is pretty dangerous.

@morris25
Copy link
Contributor Author

The docs have been updated for a while now if anyone has time for re-review.

@KristofferC KristofferC reopened this Sep 22, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2021

Codecov Report

Merging #135 (3c49843) into master (90a5ee5) will decrease coverage by 4.86%.
The diff coverage is 81.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage   93.10%   88.23%   -4.87%     
==========================================
  Files           4        5       +1     
  Lines         377      408      +31     
==========================================
+ Hits          351      360       +9     
- Misses         26       48      +22     
Impacted Files Coverage Δ
src/TimerOutputs.jl 100.00% <ø> (ø)
src/precompile.jl 0.00% <0.00%> (ø)
src/TimerOutput.jl 86.69% <95.00%> (-3.86%) ⬇️
src/show.jl 95.49% <100.00%> (+0.16%) ⬆️
src/utilities.jl 95.23% <0.00%> (-1.59%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3a4dde...3c49843. Read the comment docs.

@fredrikekre
Copy link
Contributor

One could perhaps suggest in the docs or even enforce that those timers are fetched using the package UUID?

@KristofferC
Copy link
Owner

One could perhaps suggest in the docs or even enforce that those timers are fetched using the package UUID?

I think it is fine to warn.

@KristofferC KristofferC merged commit 7be1209 into KristofferC:master Sep 22, 2021
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.

Feature Request: Registered Timers
5 participants