Cache validity of a locale name #1254
Open
+4
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently `_cache` is only populated on `load`, this is an issue with code which do a lot of locale parsing / instantiation but for one reason or an other rarely end up needing to actually load the locale data (e.g. date formatting with non-locale-dependent patterns) because every cache miss is an `os.path.exists`. This change takes advantage of semantics differences between `exists` and `load`: `exists` just needs the entry to exist in the cache, while `load` needs the entry to be truthy. Thus `exists` can cache its lookup by setting an empty dict (or even `None` but that seems a bit too dodgy). Alternatively we could remove the `os.path.exists` "slow path" and call `normalize_locale` every time, as `locale_identifiers` gets cached on first call, the initial call would be a lot more expensive but then no further `exists` would need to touch the filesystem. For a third alternative, `exists` could be decorated with `functools.cache`, in order to have its own cache completely independent of `load`. `load` could also be migrated to `functools.cache` as, as far as I can tell, the `localedata._cache` is never invalidated anyway, with the drawback that a locale's data might be resolved multiple times if concurrent calls are tight enough (`functools.cache` synchronises the cache but not the resolution of the value).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1254 +/- ##
=======================================
Coverage 92.04% 92.05%
=======================================
Files 27 27
Lines 4715 4718 +3
=======================================
+ Hits 4340 4343 +3
Misses 375 375
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently
_cacheis only populated onload, this is an issue with code which do a lot of locale parsing / instantiation but for one reason or an other rarely end up needing to actually load the locale data (e.g. date formatting with non-locale-dependent patterns) because every cache miss is anos.path.exists.This change takes advantage of semantics differences between
existsandload:existsjust needs the entry to exist in the cache, whileloadneeds the entry to be truthy. Thusexistscan cache its lookup by setting an empty dict (or evenNonebut that seems a bit too dodgy).Alternatively we could remove the
os.path.exists"slow path" and callnormalize_localeevery time, aslocale_identifiersgets cached on first call, the initial call would be a lot more expensive but then no furtherexistswould need to touch the filesystem.For a third alternative,
existscould be decorated withfunctools.cache, in order to have its own cache completely independent ofload.loadcould also be migrated tofunctools.cacheas, as far as I can tell, thelocaledata._cacheis never invalidated anyway, with the drawback that a locale's data might be resolved multiple times if concurrent calls are tight enough (functools.cachesynchronises the cache but not the resolution of the value).