Open noizu opened 5 years ago
Hi
It would be nice to improve performance. A few thoughts:
Noted. I can make that change
The cache key is linked to the current time zone database so old data is invalidated as the key changes (although old values should be removed.)
When I have some downtime after the Christmas rush i'll prepare a pull request that makes it an optional behavior and removes the hard dependency.
Hi. cache key linked to the time zone database version is good 👍 Just a heads up: I plan to merge something like this PR very soon (but with slight changes): https://github.com/lau/tzdata/pull/64 I will follow up once it's merged. This change will mean that instead of returning a list of all periods for a zone, only zero, one or two periods will be returned. Depending on the datetime you are querying for.
This will mean changes to caching because database version + time zone name will no longer be enough as a key. Also the datetime will matter. It doesn't make much sense to cache for only a specific second (and time zone and time zone database). Instead it would make sense to cache based on a range of datetimes - for instance for America/New_York during all of summer when there is no gap or overlap. But for the "fall back" period of one hour in November where clocks are set back, two periods will be returned. And then for winter there is a long period with one period of no DST until the "spring forward" event.
Noted I'll keep an eye out for the change and push something mid to late January.
About fastglobal: OTP 22 will ship with something called persistent_term
, which is aimed at usecases like these: It's global, not garbage collected, and has no locks so it's faster than ETS.
https://github.com/erlang/otp/pull/1989
We could probably wait for OTP 22 to ship, then raise the minimum required OTP version.
Edit: Oh actually, it shipped in 21.2 already!
Sounds good. I put in a request to get work approval to prepare a pull request for this. Hopefully within the next few weeks. Will have to rethink some things do to the database changes.
@archseer 👍 using persistent_term sounds like a better idea than adding dependencies.
@noizu Cool. A simple way of doing it could be to fetch and cache a list of all periods for each zone when using caching. Essentially how it was before the change to ETS. The code today can take any amount of "possible periods" and will filter them to get the right 1 or 2 periods. Today with the recent change those possible periods are just the exact ones needed fetched via ETS queries mean that the "possible periods" are the actual periods - unless the requested time is in the "far future". But because in previous versions there would be one long list the code can still use one long list. This caching of a long list with persistent_term could possibly be faster than ETS.
An alternative to a long list would be another data structure that might be faster - maybe a kind of tree. But for this maybe there need to be separate trees for "wall time" queries and UTC queries.
I'll definitely investigate it. Likely what I'll wind up doing is defining a lookup time zone data behavior and adding the ability to specify which provider to use at compile time, while providing the existing no cache provider and an additional provider that uses something like persistent_term
.
If persistent_term itself isn't fast enough for my uses (since i do billions and billions of calculations a day) I can then provide privately or publicly an additional provider that uses the precompiled module approach used by fast global directly or by including that library.
I wonder, have you tried to benchmark persistent_term
based implementation first? It was specifically designed to replace mochiglobal/precompiled module type data stores, so it should perform about as fast. https://github.com/erlang/otp/pull/1989#issuecomment-430632381
I've been experiencing some issues with the speed of tzdata
lookups, so I thought I'd quickly throw together a rudimentary persistent_term
solution and compare it against the ets
version and @noizu's fastglobal
solution. Here's my benchmark results against a "real world" use-case:
Name ips average deviation median 99th %
default ets 2.94 K 339.95 μs ±21.43% 328 μs 578 μs
fastglobal 3.21 K 311.65 μs ±23.12% 295.98 μs 518.55 μs
persistent_term 3.41 K 293.66 μs ±23.79% 277.98 μs 496.87 μs
So, it seems persistent_term
is 15-20% faster in my use-case, though this doesn't compare period lookups.
However, if you read the best practices for using persistent terms you can see there's a big overhead for erasing terms. I've not gone too deep into tzdata's update process, but if the data needs to be first erased and reinserted rather than simply overwritten, moving to persistent-term could cause issues for large applications.
I think in practice the frequency of rewrites shouldn't be significant. FastGlobal has a similiar over head I believe and despite running on a large code base with a lot more complex objects than just tz data cached on FG it hasn't been a problem. At a minimum It shouldn't be worse off for a ets constrained solution than the default.
In my use case at least the performance boost is verging on an order of a magnitude but I do billions of those time zone calculations which wouldn't be the case for most apps.
I do millions of date time calculations per minute and ets begins to become a bottle neck after a while. I just spun up a temporary work around using fast global and the semapahore library from discord, i'd love to see something like this make it's way into the library propery, preferably in a way that reduces the number of atoms required.
If you welcome the change and have directives I can begin going over the codebase more and release a more well designed solution for inclusion in future builds.
https://github.com/lau/tzdata/compare/master...noizu:fg_opt