Minimize allocations when unpacking TimeZones from cache
#423
Conversation
Benchmark Report for ~/repos/TimeZones.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTargetBaseline |
|
Re-ran the benchmarks and the ones that are ❌ above are ✅ on re-run (and others are now not significant), so i think they're very noisy? Especially as most of these don't look like they even hit the codepaths changed in this PR? Anyway, from this doesn't seem like we can say the two caches cause any performance issue |
src/types/timezone.jl
Outdated
| tz, class = get!(_tz_cache(), str) do | ||
| tz_path = joinpath(_COMPILED_DIR[], split(str, "/")...) | ||
| ftz, class = get(_ftz_cache(), str, (nothing, Class(:NONE))) | ||
| if ftz !== nothing | ||
| _check_class(mask, class) | ||
| return ftz::FixedTimeZone | ||
| end |
There was a problem hiding this comment.
🤔 hrrmm the downside of the https://github.com/JuliaConcurrent/MultiThreadedCaches.jl approach is that it currently only exposes get!() as its interface, for reasons that i cannot fully remember..
So the trick you're doing here wouldn't work.
For one workaround idea, maybe you could figure out which type of TimeZone it is ahead of time? I don't know if that's possible though, just from its name? I guess you could have a third cache that just caches the type of the timezone?
So it'd be something like:
type = get!(_tz_type_cache, str) do
compute_tz_type(str)
end
if type === :FixedTimeZone
tz, class = get!(ftz_cache, str) do
compute_fixed_timezone(str)
end
elseif type === :VariableTimeZone
tz, class = get!(vtz_cache, str) do
compute_variable_timezone(str)
end
endSomething like that? I'm not sure if splitting things up this way is feasible though?
There was a problem hiding this comment.
figure out which type of TimeZone it is ahead of time?
I think that's what's not easy, because we actually create the TimeZones by reading from a timezone-specific file
TimeZones.jl/src/tzjfile/read.jl
Lines 52 to 78 in f8f39b3
i.e. we have to read the file to know what type it will be, and we don't want to have to read multiple times
so i think we'd have change that read function to return the type, then put that value in a third cache, while holding on to the tz, class
something like
local tz, class
type = get!(_tz_type_cache, str) do
tz, class, type = compute_tz(str)
return type
end
if type === :FixedTimeZone
tz, class = get!(ftz_cache, str) do
(tz, class)
end
elseif type === :VariableTimeZone
tz, class = get!(vtz_cache, str) do
(tz, class)
end
endThere was a problem hiding this comment.
hmmmm, this way's not looking nice to me 🤔
how much benefit do we think MultiThreadedCaches.jl is going to add for us?
There was a problem hiding this comment.
we could maybe go add get and empty! to the MultiThreadedCaches.jl API to reduce the necessary changes
There was a problem hiding this comment.
yeah, i don't see a nice way to re-write this using MultiThreadedCaches.jl
at least not without extending the MultiThreadedCaches.jl API a bit, which feels like it should be done separately and not block this PR (unless we're worried about correctness issues in this PR as is)
There was a problem hiding this comment.
👍 👍 This is a nice change, @nickrobinson251! It makes sense why it reduces the allocations. 👍 Thanks for investigating this so deeply
|
I'm not sure i understand why the VariableTimeZones still alloc when you look them up. I agree with you it is unexpected.. I'm not sure the isbits thing should matter, since we're pointing to the existing vector, not allocating a new one, and Julia is supposed to be able to stack-allocate an immutable struct that contains mutable fields, since like 1.4 or something. So i'm not sure i understand yet; it might be good to look through a profile together, once the rest of the issues in the PR are addressed. |
|
Okay, i've spent a couple days playing around with different options but i can't yet see how to remove the remaining VariableTimeZones allocation, so i propose leaving that as a follow-up. And likewise i don't think a switch to MultiThreadedCaches.jl is straight forward, so would prefer to get this perf improvement in and leave that refactor to a follow-up. @NHDaly and @omus please could you take another look at this PR? Thanks! |
|
I'll try to take a look soon. I'll call out that I also want to get #382 merged soon as well. |
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## master #423 +/- ##
==========================================
- Coverage 95.58% 95.11% -0.47%
==========================================
Files 38 36 -2
Lines 1766 1803 +37
==========================================
+ Hits 1688 1715 +27
- Misses 78 88 +10
|
|
I've been a bit swamped this week. Overall, I'm in favour of this change and will definitely merge this before #382 |
|
Just following up to make sure this doesn't drop off your radar. Let me know if i can do anything to help, @omus :) |
- Should not allocate for cached FixedTimeZone - Should allocate only once for cached VariableTimeZone
- To minimize allocations when unpacking TimeZones from the cache
Co-authored-by: Nathan Daly <nathan.daly@relational.ai>
nickrobinson251
force-pushed
the
npr/timezone-allocations
branch
from
718ed72
to
b158178
Compare
|
It turned out #382 became urgent to merge for Julia 1.9 so the original plan had changed. If @nickrobinson251 you have time to rebase this against the latest version of the code that would be great. If not, I'll try to adapt this code myself sometime. |
|
Is this just a (simple) rebase away? It seems good as is, down to 0 allocations is great, already an improvement. Getting rid of the 1 remaining alloc for VariableTimeZone would be ideal, but shouldn't stop from merging as is? I didn't look closely into why it's there, maybe fixed already on 1.10? |
|
I think this is just a (not-very-simple) rebase away. And I would love, love, love someone to take that on, as I don't have capacity at the moment -- please feel encouraged to do so! |
|
Superseded by #451 |
TimeZonefrom string allocates unnecessarily #422Even though we're careful about caching the creation of TimeZones and made
FixedTimeZoneanisbitstype/allocatedinline, we still allocate everytimeTimeZoneis called, even if just returning aFixedTimeZonefrom the cache!I think this happens because the values in the cache are of type
TimeZone, i.e. it's not known if we're returning aFixedTimeZoneor aVariableTimeZone.This PR proposes to instead maintain separate caches (per thread) for
FixedTimeZones andVariableTimeZones, rather than a single one (per thread) for allTimeZonesEffectively this is some code duplication for a performance improvement.
VariableTimeZonestill allocates once, presumably since it's notisbitstype(#271).Micro benchmarks
FixedTimeZoneThis PR
master(v1.9.1)VariableTimeZoneThis PR
master(v1.9.1)