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

ets bottle neck. #69

Open
noizu opened this issue Dec 4, 2018 · 12 comments
Open

ets bottle neck. #69

noizu opened this issue Dec 4, 2018 · 12 comments

Comments

@noizu
Copy link

noizu commented Dec 4, 2018

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.

master...noizu:fg_opt

@lau
Copy link
Owner

lau commented Dec 4, 2018

Hi

It would be nice to improve performance. A few thoughts:

  • I am reluctant to add new dependencies. Perhaps fastglobal could be added as an optional dependency then people could add fastglobal and then Tzdata could make use of it if present. Maybe with a config setting to indicate that use of fastglobal is wanted.
  • It is important that cached data is not being used after switching to a new version of the tz database in runtime.

@noizu
Copy link
Author

noizu commented Dec 4, 2018

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.

@lau
Copy link
Owner

lau commented Dec 26, 2018

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): #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.

@noizu
Copy link
Author

noizu commented Dec 26, 2018

Noted I'll keep an eye out for the change and push something mid to late January.

@archseer
Copy link

archseer commented Jan 4, 2019

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.

erlang/otp#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!

@noizu
Copy link
Author

noizu commented Mar 10, 2019

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.

@lau
Copy link
Owner

lau commented Mar 10, 2019

@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.

@noizu
Copy link
Author

noizu commented Mar 10, 2019

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.

@archseer
Copy link

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. erlang/otp#1989 (comment)

@OldhamMade
Copy link

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.

@noizu
Copy link
Author

noizu commented Nov 13, 2019

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.

@noizu
Copy link
Author

noizu commented Nov 13, 2019

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.

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

No branches or pull requests

4 participants