mattjohnsonpint / TimeZoneConverter

Lightweight libraries to convert between IANA, Windows, Rails, and POSIX time zones.
Other
842 stars 81 forks source link

Poor startup performance due to static constructor + decompression #135

Open alex-jitbit opened 1 year ago

alex-jitbit commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

DenisSikic commented 1 year ago

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 commented 1 year ago

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!