-
Notifications
You must be signed in to change notification settings - Fork 54
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
Lazily initialize the time zone cache #457
Conversation
src/types/timezone.jl
Outdated
_COMPILED_DIR[] = if desired_version == TZJData.TZDATA_VERSION | ||
TZJData.ARTIFACT_DIR | ||
else | ||
# @info "Loading tzdata $desired_version" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented out this because otherwise it just logs randomly when you call a function and the cache initializes. It feels a bit pointless in the first place as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The point was to alert users that the environmental JULIA_TZ_VERSION
was specified to a different version than TimeZones.jl would use by default. I'm not opposed to dropping this but it was informative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay but it will just appear at some random point now without any context to the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a reasonable compromise is to display a message during the __init__
and perform the loading like you propose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I'm good with this. Just a couple of things to clean up
this avoids having to compile a bunch of code in `__init__` and improves the load time of the package (on Julia 1.11) from: ```julia julia> @time_imports import TimeZones ... ┌ 2.6 ms TimeZones.TZData.__init__() 92.83% compilation time ├ 237.9 ms TimeZones.__init__() 94.11% compilation time (89% recompilation) 276.3 ms TimeZones 81.90% compilation time (88% recompilation) ``` ```julia julia> @time_imports import TimeZones ... 17.5 ms TimeZones 16.03% compilation time ``` The performance of the functions seem more or less unaffected: Before: ```julia julia> using BenchmarkTools, TimeZones julia> @Btime istimezone("Europe/Warsaw") 48.068 ns (1 allocation: 16 bytes) true julia> @Btime TimeZone("Europe/Warsaw") 28.591 ns (2 allocations: 64 bytes) Europe/Warsaw (UTC+1/UTC+2) ``` After: ```julia julia> @Btime istimezone("Europe/Warsaw") 45.356 ns (1 allocation: 16 bytes) true julia> @Btime TimeZone("Europe/Warsaw") 28.687 ns (2 allocations: 64 bytes) Europe/Warsaw (UTC+1/UTC+2) ```
Rebased this PR now that #456 has been merged |
Posting some benchmarks from my machine so I can do some refactoring without losing the performance gains. Before this PR with Julia 1.11.0-beta1 on an M1 MacBook julia> @time_imports import TimeZones
...
┌ 4.4 ms TimeZones.TZData.__init__() 53.17% compilation time
├ 318.6 ms TimeZones.__init__() 85.10% compilation time (93% recompilation)
690.8 ms TimeZones 59.42% compilation time (85% recompilation)
julia> using BenchmarkTools, TimeZones
julia> @btime istimezone("Europe/Warsaw");
77.314 ns (1 allocation: 16 bytes) With this PR with Julia 1.11.0-beta1 on an M1 MacBook julia> @time_imports import TimeZones
...
┌ 3.7 ms TimeZones.TZData.__init__() 56.80% compilation time
├ 0.0 ms TimeZones.__init__()
107.0 ms TimeZones 83.43% compilation time (53% recompilation)
julia> using BenchmarkTools, TimeZones
julia> @btime istimezone("Europe/Warsaw");
80.966 ns (1 allocation: 16 bytes) |
Iterating on the `istimezone` internalsIteration 1function istimezone(str::AbstractString, mask::Class=Class(:DEFAULT))
# Start by performing quick FIXED class test
if mask & Class(:FIXED) != Class(:NONE) && occursin(FIXED_TIME_ZONE_REGEX, str)
return true
end
# Checks against pre-compiled time zones
tz, class = get(get_tz_cache(), str, (nothing, Class(:NONE)))
return tz !== nothing && mask & class != Class(:NONE)
end julia> @time_imports import TimeZones
...
┌ 2.7 ms TimeZones.TZData.__init__() 77.28% compilation time
├ 0.0 ms TimeZones.__init__()
106.6 ms TimeZones 84.49% compilation time (53% recompilation)
julia> using BenchmarkTools, TimeZones
julia> @btime istimezone("Europe/Warsaw");
95.032 ns (2 allocations: 64 bytes) Iteration 2function istimezone(str::AbstractString, mask::Class=Class(:DEFAULT))
# Start by performing quick FIXED class test
if mask & Class(:FIXED) != Class(:NONE) && occursin(FIXED_TIME_ZONE_REGEX, str)
return true
end
# Checks against pre-compiled time zones
tz_class = get(get_tz_cache(), str, nothing)
tz_class === nothing && return false
tz, class = tz_class
return mask & class != Class(:NONE)
end julia> @time_imports import TimeZones
...
┌ 2.5 ms TimeZones.TZData.__init__() 78.26% compilation time
├ 0.0 ms TimeZones.__init__()
103.5 ms TimeZones 83.75% compilation time (52% recompilation)
julia> using BenchmarkTools, TimeZones
julia> @btime istimezone("Europe/Warsaw");
75.789 ns (1 allocation: 16 bytes) Iteration 3function istimezone(str::AbstractString, mask::Class=Class(:DEFAULT))
# Start by performing quick FIXED class test
if mask & Class(:FIXED) != Class(:NONE) && occursin(FIXED_TIME_ZONE_REGEX, str)
return true
end
# Checks against pre-compiled time zones
_, class = get(get_tz_cache(), str, (UTC_ZERO, Class(:NONE)))
return mask & class != Class(:NONE)
end julia> @time_imports import TimeZones
...
┌ 3.1 ms TimeZones.TZData.__init__() 66.09% compilation time
├ 0.0 ms TimeZones.__init__()
103.3 ms TimeZones 83.89% compilation time (52% recompilation)
julia> using BenchmarkTools, TimeZones
julia> @btime istimezone("Europe/Warsaw");
80.492 ns (1 allocation: 16 bytes) Iteration 4function istimezone(str::AbstractString, mask::Class=Class(:DEFAULT))
# Start by performing quick FIXED class test
if mask & Class(:FIXED) != Class(:NONE) && occursin(FIXED_TIME_ZONE_REGEX, str)
return true
end
# Checks against pre-compiled time zones
class = get(get_tz_cache(), str, (UTC_ZERO, Class(:NONE)))[2]
return mask & class != Class(:NONE)
end julia> @time_imports import TimeZones
...
┌ 2.8 ms TimeZones.TZData.__init__() 79.89% compilation time
├ 0.0 ms TimeZones.__init__()
102.1 ms TimeZones 83.99% compilation time (54% recompilation)
julia> using BenchmarkTools, TimeZones
julia> @btime istimezone("Europe/Warsaw");
75.610 ns (1 allocation: 16 bytes) |
Commit 5701f3a julia> @time_imports import TimeZones
...
┌ 3.1 ms TimeZones.TZData.__init__() 77.56% compilation time
├ 0.0 ms TimeZones.__init__()
106.5 ms TimeZones 84.26% compilation time (54% recompilation)
julia> using BenchmarkTools, TimeZones
julia> @btime istimezone("Europe/Warsaw");
78.689 ns (1 allocation: 16 bytes) Commit b5948fc julia> @time_imports import TimeZones
...
0.6 ms Mocking
┌ 2.5 ms TimeZones.TZData.__init__() 81.56% compilation time
├ 0.0 ms TimeZones.__init__()
108.0 ms TimeZones 79.75% compilation time (53% recompilation)
julia> using BenchmarkTools, TimeZones
julia> @btime istimezone("Europe/Warsaw");
75.703 ns (1 allocation: 16 bytes) Looks like I broke some tests in the process |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #457 +/- ##
==========================================
- Coverage 92.79% 92.67% -0.12%
==========================================
Files 39 39
Lines 1818 1830 +12
==========================================
+ Hits 1687 1696 +9
- Misses 131 134 +3 ☔ View full report in Codecov by Sentry. |
Close to the finish line on this one. I'd like to recheck the performance with the fixed tests yet. |
This avoids having to compile a bunch of code in
__init__
and improves the load time of the package (on Julia 1.11) from:The change in performance of the functions is (in my opinion) acceptable.
After:
Before: