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

Introduced thread-affine CultureInfo cache #1033

Merged
merged 1 commit into from
Feb 27, 2017

Conversation

mrange
Copy link
Contributor

@mrange mrange commented Feb 27, 2017

Caches CultureInfo per Thread in order to improve DateTime parsing when parsing with specific Culture

The idea for this optimization started from this StackOverflow question: https://stackoverflow.com/questions/42469637/

Performance numbers:

Parsing 100000 dates
Using cached CultureInfo object...
took 114 ms
Using updated TextRuntime.GetCulture...
took 144 ms
Using fresh CultureInfo object (same technique as old TextRuntime.GetCulture)...
took 52556 ms

Caches CultureInfo per Thread in order to improve DateTime parsing when parsing with specific Culture

The idea for this optimization started from this StackOverflow question: https://stackoverflow.com/questions/42469637/

Performance numbers:

Parsing 100000 dates
Using cached CultureInfo object...
  took 114 ms
Using updated TextRuntime.GetCulture...
  took 144 ms
Using fresh CultureInfo object (same technique as old TextRuntime.GetCulture)...
  took 52556 ms
@ovatsus ovatsus merged commit 05261d6 into fsprojects:master Feb 27, 2017
@ovatsus
Copy link

ovatsus commented Feb 27, 2017

Thanks

@mrange
Copy link
Contributor Author

mrange commented Feb 27, 2017

That was quick... I just like to point out that since I have gone all in VS2017 I haven't been able to compile the type provider locally. However, I have tested the functions that were causing problems in the stackoverflow post and they seem to perform a lot faster now (~50x)

@rmunn
Copy link

rmunn commented Feb 28, 2017

Another possible solution might be to leverage the CultureInfo.GetCultureInfo static method, which is documented as being cached already, avoiding the need to create and use our own cache. The only pitfall with that method is that it will return the invariant culture if its argument is an empty string, but it will throw an exception (!) if its argument is null. (Why, C#, why?!?)

So a simple defaultArg cultureStr "" followed by a call to CultureInfo.GetCultureInfo should be enough, and it would certainly be simpler than the "implement our own cache" taken in this PR.

@mrange
Copy link
Contributor Author

mrange commented Feb 28, 2017

CultureInfo.GetCultureInfo uses a synchronized HashTable under the hood but looking more closely at the implementation reveals that lookups are look-free which is good. "Unfortunately" it also does .AnsiToLower which will add some kind of GC pressure but probably not that much.

So I think it makes sense to use CultureInfo.GetCultureInfo instead however in order to preserve the semantics of TextRuntime.GetCulture we still need the test String.IsNullOrWhiteSpace cultureStr because this throws: CultureInfo.GetCultureInfo " " where this returns invariant culture: TextRuntime.GetCulture " "

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

Successfully merging this pull request may close these issues.

None yet

3 participants