search

JuliaTime / TimeZones.jl

IANA time zone database access for the Julia programming language
Other
86 stars 52 forks source link

Make Time Zone Cache thread-safe with lazily-allocated thread-local caches #344

Closed NHDaly closed 3 years ago

NHDaly commented 3 years ago

Make Time Zone Cache thread-safe with lazily-allocated thread-local caches. Each OS-thread has its own thread-local Time Zone Cache, which makes it thread-safe to construct TimeZones simultaneously from multiple threads.

Fixes https://github.com/JuliaTime/TimeZones.jl/issues/342.

I coded this through the GitHub UI during a meeting... so I haven't tested it locally. Sorry about that, but I just wanted to get this started. :)

NHDaly commented 3 years ago

It would be great to make a test to ensure we don't break thread-safety again

Agreed, but i couldn't figure out how to write a test like that. It seems to be a pretty nondeterministic failure, and I didn't manage to get it to reproduce in a small unit test. Open to suggestions from others!! @comnik: Are you able to suggest anything?

NHDaly commented 3 years ago

Okay, I've added a small test that fails about 75% of the time for me (when run on master; obviously never fails on this PR), and takes around 10seconds to run. Hopefully that's an acceptable tradeoff! You might have some ideas for how to increase the likelihood of failure since you know the code better, @omus

omus commented 3 years ago

Thanks! I'll try to do some performance testing against #345 tonight

omus commented 3 years ago

@omus, did your testing bear out happily? :)

Slowly working on them. I have found a readers-writer lock is working pretty well without adding the extra storage requirements introduced here

omus commented 3 years ago

I'm good to go ahead with this implementation. Performance comparison can be found here. I had other implementations but they were also slower so I left them out. Just need some refactoring here and we can proceed.

NHDaly commented 3 years ago

Okay! :) Thanks @omus - please take another look. I've resolved the compile() cache clearing, and it was actually pretty straightforward 👍 Thanks!

omus commented 3 years ago

I think we'll need to do some special casing for Julia 1.0 support. Update: we just need to not use julia -t on Julia 1.0

codecov-commenter commented 3 years ago

Codecov Report

Merging #344 (bcef249) into master (3e78a3e) will increase coverage by 0.05%. The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #344      +/-   ##
==========================================
+ Coverage   93.85%   93.90%   +0.05%     
==========================================
  Files          32       32              
  Lines        1529     1543      +14     
==========================================
+ Hits         1435     1449      +14     
  Misses         94       94
Impacted Files Coverage Δ
src/types/timezone.jl 91.66% <92.85%> (+3.66%) :arrow_up:
src/TimeZones.jl 100.00% <100.00%> (ø)
src/build.jl 100.00% <100.00%> (ø)
src/tzdata/compile.jl 95.53% <100.00%> (+0.03%) :arrow_up:

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 3e78a3e...bcef249. Read the comment docs.

omus commented 3 years ago

I was waiting to tag this due to https://github.com/JuliaTime/TimeZones.jl/pull/356 but I think I'll go ahead and make a release anyway. We can always make more :)

NHDaly commented 3 years ago

I was waiting to tag this due to #356 but I think I'll go ahead and make a release anyway. We can always make more :)

Yeah agreed, that sounds like the right move to me, too! :)

NHDaly commented 3 years ago

Thanks Curtis! :)

Sacha0 commented 3 years ago

🎉 ! :)