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

Poor startup performance due to static constructor + decompression #135

Open
alex-jitbit opened this issue Feb 4, 2023 · 6 comments
Open

Comments

@alex-jitbit
Copy link

alex-jitbit commented Feb 4, 2023

First of all, thank you (again) for this wonderful library that saves me every freaking day! ❤❤❤

This is not a bug, but a suggestion.

You have a static constructor that takes from 50 to 72 milliseconds in my benchmarks. I suspect that's because of the (a) decompression of embedded archives and (b) reading from windows registry

Static constructors are bad enough already (all threads are blocked while it executes exclusively, they pollute exceptions, can sometimes cause deadlocks, etc. ) and having some heavy-computing in it is not the best idea.

This becomes worse when an ASP.NET Core app is restarting/recycling.

I suggest including plain, decompressed CSV files into the dll. I checked the sizes, the biggest file is 20KB, which is fine, the dll is already 35KB, no big deal.

If you're OK with this I will start working on a PR

P.S. Also maybe explore lazyloading instead of static constructors where possible.

@mattjohnsonpint
Copy link
Owner

mattjohnsonpint commented Feb 12, 2023

Hi. Thanks for bringing this up.

Though I could skip the decompression step as you suggested, I would still need to read the files into their data structures during static initialization. The runtime performance is much more important than the initialization time (IMHO) - which is why the data is loaded into dictionaries for fast lookup.

I suspect the decompression time vs. size is not that big of a trade-off. If you wanted to add some Benchmark.NET tests to check, that would be informative.

As far as lazy vs static, that might be cleaner in code, but would have the same effect - the data would be loaded on first use.

To reduce request time in an ASP.NET Core app, you could consider invoking one of the methods during your startup. That would move the initialization cost to the app startup instead of a request.

@DenisSikic
Copy link

Hi Guys, I'm writing a blazor application and am wondering if you have found a workaround? It would be good to have something I can call during startup that loads these resources on a separate thread, as opposed to blocking all threads because of the static constructor. Thanks.

@mattjohnsonpint
Copy link
Owner

@DenisSikic - You can call any of the current APIs during your initialization. That will invoke the static constructor internally.

For example, you could call TZConvert.KnownIanaTimeZoneNames or TZConvert.IanaToWindows("America/New_York"), even if you didn't need to do anything with the result.

@mattjohnsonpint
Copy link
Owner

If you need it async, you could put it in a Task.Run and don't await the result.

@DenisSikic
Copy link

Can you give me a method to call that isn't in that static constructor? It doesn't do any good to call from Task.Run if static constructors block all threads and can cause deadlocks.

If you give us something to call and then just check in the static constructor IsLoaded, then we're good to go.

@mattjohnsonpint
Copy link
Owner

Ok, I think I understand the concern.

Perhaps a better design would be to lazy-load using Lazy<T> in the implementation. I could also provide an Initialize() method on the public API for those that want to take the hit earlier in app initialization. I can work on that.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants